feat(accordion): added support for leading icon#535
Conversation
🦋 Changeset detectedLatest commit: dec0116 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Adds “leading icon” support for accordion headers by extending the underlying details markup/API and updating Skin styles and docs to accommodate the new layout.
Changes:
- Adds a
leadingslot/prop toebay-details(Marko + React) and demonstrates it in accordion examples/stories. - Updates Skin accordion/details CSS to support a leading icon and replaces
space-betweenwith a flex-grow label layout. - Updates documentation pages, generated dist CSS, snapshots, and adds new accordion icon story/tests.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/_index/components/details/css+page.marko | Adds a “details with leading icon” documentation example and minor whitespace fixes. |
| src/routes/_index/components/accordion/css+page.marko | Adds a leading-icon accordion demo section and code samples. |
| packages/skin/src/sass/details/details.scss | Adds gap to the summary layout and removes icon margin spacing. |
| packages/skin/src/sass/accordion/accordion.scss | Changes accordion summary layout to rely on label flexing instead of space-between. |
| packages/skin/src/sass/accordion/stories/accordion.stories.js | Adds a new Skin story demonstrating leading icons in accordion. |
| packages/skin/package.json | Changes Skin start script behavior. |
| packages/skin/dist/details/details.css | Regenerated dist CSS for details summary gap/margin removal. |
| packages/skin/dist/accordion/accordion.css | Regenerated dist CSS for accordion label flex rule. |
| packages/ebayui-core/src/components/ebay-details/index.marko | Adds leading attr-tag rendering to Marko ebay-details. |
| packages/ebayui-core/src/components/ebay-details/component-browser.ts | Exposes leading in the Marko input typings. |
| packages/ebayui-core/src/components/ebay-details/test/snapshots/test.server.js.snap | Updates snapshots (primarily Marko key changes). |
| packages/ebayui-core/src/components/ebay-accordion/examples/withIcon.marko | New Marko example using @leading within accordion items. |
| packages/ebayui-core/src/components/ebay-accordion/accordion.stories.ts | Adds a new WithIcon Storybook story. |
| packages/ebayui-core/src/components/ebay-accordion/test/test.server.js | Adds snapshot test coverage for the new WithIcon story. |
| packages/ebayui-core/src/components/ebay-accordion/test/snapshots/test.server.js.snap | Adds snapshot output for the icon story. |
| packages/ebayui-core/src/components/ebay-accordion/test/snapshots/test.browser.js.snap | Updates browser snapshots due to CSS layout changes. |
| packages/ebayui-core-react/src/ebay-details/ebay-details.tsx | Adds leading prop rendering for React EbayDetails. |
| packages/ebayui-core-react/src/ebay-accordion/tests/index.stories.tsx | Adds a React accordion story demonstrating leading icons. |
| packages/ebayui-core-react/src/ebay-accordion/tests/render.spec.tsx | Adds a snapshot test for the new leading-icon accordion story. |
| packages/ebayui-core-react/src/ebay-accordion/tests/snapshots/render.spec.tsx.snap | Adds snapshot output for the leading-icon accordion story. |
| .changeset/big-breads-jog.md | Minor version bumps for Skin, Marko core, and React core. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
LuLaValva
left a comment
There was a problem hiding this comment.
This is probably good to merge, the nitpick I have can be ignored.
Only thought— Is there any reason you decided to be more generic with leading instead of leadingIcon? Are there plans to add leading text or image?
| > { | ||
| summary?: Marko.AttrTag<Marko.Input<"span">>; | ||
| leading?: Marko.AttrTag<Marko.Input<"span">>; | ||
| size?: "regular" | "small"; |
There was a problem hiding this comment.
We have better types for this now! Use Marko.HTML.Span
There was a problem hiding this comment.
Will ignore it since at least how we have it its consistent with the rest of the tag.
In evo-marko we should use the new one.
I went leading because its more generic. So its mainly just so its consistent with other components. |
|
@claude can you review this PR? |
Description
Screenshots
Large
Small
Checklist