Skip to content
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

feat(accordion): new component #2497

Merged
merged 7 commits into from
Nov 22, 2024
Merged

feat(accordion): new component #2497

merged 7 commits into from
Nov 22, 2024

Conversation

saiponnada
Copy link
Contributor

@saiponnada saiponnada commented Nov 14, 2024

Fixes #2476

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

  • Added new accordion component that uses details under the hood.

Notes

  • The auto-collapse feature on the skin uses an attribute name that does not support our full browser policy. This configuration will be implemented using JavaScript (on eBay UI).

Screenshots

image

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

Copy link

changeset-bot bot commented Nov 14, 2024

🦋 Changeset detected

Latest commit: 51ba760

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ebay/skin Minor

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

Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial thoughts...

src/sass/details-group/details-group.scss Outdated Show resolved Hide resolved
dist/details-group/details-group.css Outdated Show resolved Hide resolved
@ianmcburnie
Copy link
Contributor

ianmcburnie commented Nov 21, 2024

Why details-group and not accordion? We should name it according to its MIND pattern counterpart: https://opensource.ebay.com/mindpatterns/disclosure/accordion/index.html

@saiponnada saiponnada changed the title feat(details-group): new component feat(accordion): new component Nov 21, 2024
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. A couple of more minor things...

src/sass/accordion/accordion.scss Outdated Show resolved Hide resolved
src/modules/accordion.marko Outdated Show resolved Hide resolved
src/sass/accordion/accordion.scss Show resolved Hide resolved
@ianmcburnie
Copy link
Contributor

Regarding role=list it appears there might be a different way to solve this issue now, see: #2499 (comment)

Copy link
Contributor

@ianmcburnie ianmcburnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some tone of voice edits to better match with Skin and MIND patterns.

src/modules/accordion.marko Outdated Show resolved Hide resolved
src/modules/accordion.marko Outdated Show resolved Hide resolved
src/modules/accordion.marko Outdated Show resolved Hide resolved
src/modules/accordion.marko Outdated Show resolved Hide resolved
src/modules/accordion.marko Outdated Show resolved Hide resolved
src/modules/accordion.marko Outdated Show resolved Hide resolved
src/modules/accordion.marko Outdated Show resolved Hide resolved
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Co-authored-by: Ian McBurnie <38065+ianmcburnie@users.noreply.github.com>
@saiponnada saiponnada merged commit dfda4a0 into 18.5.0 Nov 22, 2024
@github-actions github-actions bot mentioned this pull request Nov 19, 2024
@ArtBlue ArtBlue deleted the accordion branch November 22, 2024 21:31
@github-actions github-actions bot mentioned this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants