Migrate accordion #2721
Migrate accordion #2721
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
GraceGardner
requested review from
rossnelson,
andrewzamojc,
Alex-Tideman and
laurakwhit
| onclick={(e) => e.stopPropagation()} | ||
| onkeyup={(e) => e.stopPropagation()} |
There was a problem hiding this comment.
I think this will break anywhere we're using click or keyup handlers like <Accordion on:click={doSomething} />. The old syntax was forwarding the listener down to this div element, this new syntax is just calling stopPropagation(), but doesn't account for if an onclick or onkeyup callback function is passed to Accordion.
There was a problem hiding this comment.
Not sure about this spot, but rather than stopping propagation, I tend to preventDefault. It sort of marks it as handled, so up the chain, a handler can check if (e.defaultPrevented).
There was a problem hiding this comment.
Oh ya know what, I read stopPropagation as preventDefault hah, so I think this is totally fine. I think we were getting double click events or something when a custom action was passed in, hence why the stopPropagation directives were added.
There was a problem hiding this comment.
@rossedfort Tested this by adding an alert in an onclick event and it seems to be working fine. I left the alert on the first accordion in storybook if you want to check?
There was a problem hiding this comment.
@GraceGardner I tested and it seems to be working. Let's remove the alert before merging though. Instead, we can add action('onclick')
There was a problem hiding this comment.
Will do, thanks Ross
| open: { control: 'boolean' }, | ||
| expandable: { control: 'boolean' }, | ||
| error: { control: 'text' }, | ||
| icon: { control: 'select', options: iconNames }, |
There was a problem hiding this comment.
Comment: Storybook might set up most of these automatically now with Meta<AccordionProps>.
There was a problem hiding this comment.
I'm a bit less sure about this one... Let me know what you think we should do here or if you have any docs that are helpful pointing me in the right direction.
There was a problem hiding this comment.
No change needed:
No worries. This is a sidetrack and doesn't need to be considered in this PR. For interest sake or future work, I found this bit on storybook choosing controls automatically https://storybook.js.org/docs/essentials/controls#choosing-the-control-type
Description & motivation 💭
Migrate accordion and accordion group to runes mode.
Note: the team has decided to leave the Accordion Light component and files using it in legacy mode for now. Will no longer be using this component in the future.
Screenshots (if applicable) 📸
Design Considerations 🎨
Testing 🧪
How was this tested 👻
Steps for others to test: 🚶🏽♂️🚶🏽♀️
Checklists
Draft Checklist
Merge Checklist
Issue(s) closed
DT-2938
Docs
Any docs updates needed?