-
Notifications
You must be signed in to change notification settings - Fork 30
feat(website): initial build of evo web #317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
agliga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theres a lot of files that are being unused. (like a src/routes/temp, all icon files, flags, market sans etc)
Lets clean those up and remove all the unneeded files. I think this will help a lot with the review since theres a lot of things that are unused.
packages/ebayui-core/src/components/ebay-card/examples/horizontal.marko
Outdated
Show resolved
Hide resolved
|
Also some icons are broken on certain links. And icons are broken on the icons link. |
I don't think I changed anything with how the icon master files were being loaded. I did see some styling issues with the icons and flags pages, and I fixed those. However, it doesn't appear as though the master files are actually loaded properly. Could this be a bug in Marko Run? @agliga @LuLaValva , can you look into it? Check out |
|
Some of the icon issues got resolved, not sure how. There are some still missing for some reason. |
agliga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, but needs some structure refactor. A lot of these patterns can be simplified and be inferred from the route.
Im happy to chat offline about this too.
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "pageTitle": "Accordion Marko Component", | |||
| "pageDescription": "The Accordion Marko component is optimized for server-side rendering and responsive streaming. It delivers a fast, accessible way to expand and collapse content while ensuring WCAG 2.1 AA compliance, semantic HTML, and seamless performance across devices.", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this repeated for each route?
It seems like a lot of extra boilerplate for each of these routes. This text is pretty much the same across all the routes except that it says marko instead of css or react.
Maybe we can have this in 1 place and add a "keyword" inside if we want it unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ideal to keep these since we get a lot of flexibility and having access to update them as needed. There are tweaks to titles and descriptions (and in the future other metas) that can be made to optimize pages and improve search engine rankings. The idea is that once these pages are live, we can track how they are doing and optimize whichever pages are not performing as well as they should to improve their visibility and rankings.
If we really want to have some pages, like the Marko and React be the same and just swap out "Marko" and "React" I suppose that's fine, but I would not want to have them all follow that pattern.
Since we only set these pages up once when we create new components, I don't think this is a big deal.
| <li>Listen to toggle event of each details element</li> | ||
| <li>When receiving a toggle event, if the details state has moved from closed to open, close all other details elements</li> | ||
| </ol> | ||
| <p>That's it. Pretty straightforward. You can reference our <a href="https://github.com/eBay/mindpatterns/blob/gh-pages/_js/accordion.js">example code on GitHub</a>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link points to mind patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. We've not yet gone through and fixed the links and done a content audit/fix. Those are one of the last steps before we launch. See the doc I mentioned in the internal chat. The doc has the list of all tasks completed and outstanding.
| <component-tabs componentInfo=metadata currentTab="A11y"> | ||
| <div id="accordion-a11y"> | ||
| <p> | ||
| <a11y-mind-pattern pattern="accordion"/> | ||
| </p> | ||
| </div> | ||
| </component-tabs> No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to be even simpler.
First off we probably should do a dynamic directory for these.
Something like:
component/$name/accessibility
In the accessibility directory, we can simply have this page
<a11y-mind-pattern pattern=$global.params.name/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this and tried a few different approaches, but I couldn't figure out a way to create a flexible solution that would allow each a11y page to have its own meta json and make the overall system more optimal. If there's a specific solution you have in mind, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the dynamic approach reduces initial boilerplate, but the individual route approach provides significantly more flexibility and better aligns with our SEO goals for component documentation. The one-time setup cost per component is worth the long-term benefits.
src/components/component-tabs.marko
Outdated
| @@ -0,0 +1,42 @@ | |||
| $ const componentInfo = input.componentInfo || {}; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe a better pattern is to have a global metadata.json file with all info across all components
Then we don't have to import metadata in the parent and we can simply import it here and infer what path we are on based on $global.url
To get the current compiled JSON you can visit https://opensource.ebay.com/evo-web/skin/metadata
We can put that in a global JSON and then expose as is, and then read it and infer which component we are on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I know DS consumes this for Playbook. As long as we're ok modifying that JSON to have some additional stuff:
{
"name": "accordion",
"playbookURL": "https://playbook.ebay.com/design-system/components/accordion",
"ds-component": {
"name": "accordion",
"staticVersion": 1.1,
"markoVersion": 1.1,
"reactVersion": 1.1
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think thats fine. Its actually better that way.
I would also make it so that if there is no markoVersion or react version then it uses static version. We don't need to add the versions if the components are all aligned.
|
Hey @ArtBlue I'm working on a PR to simplify some of the structure here, but in doing so I noticed a few problems that should be addressed separately. Please take a look:
|
Thanks, @LuLaValva !
|
|
I wish regular comments had threads 😢. Here's a reply to a reply:
|
…d one, created a failsafe for non-existent a11y patterns.
…ndering to markdown parsing, minor styling updates to tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements the initial build of the Evo Web unified website by removing numerous component documentation pages and their associated metadata files from the skin package. The changes consolidate the documentation structure as part of a larger website reorganization effort.
Reviewed changes
Copilot reviewed 106 out of 2518 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Multiple +page.marko files |
Removed component documentation pages for 40+ UI components including floating-label, flag, filter-input, field, checkbox, breadcrumbs, etc. |
Multiple +meta.json files |
Removed corresponding metadata files containing page titles and descriptions for each deleted component page |
+page.component-browser.js |
Removed browser-side component script for filter-chip interactions |
Description
A unified build of the Evo Web site
Notes
All details in offline docs.