feat(components): add a post-collapsible-trigger #3209
Conversation
🦋 Changeset detectedLatest commit: cbe8e03 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
alizedebray
force-pushed
the
3208-create-post-collapsible-trigger-component
branch
from
22f6e84
to
6b8095d
Compare
alizedebray
force-pushed
the
3208-create-post-collapsible-trigger-component
branch
from
fb37469
to
7e1bf77
Compare
alizedebray
force-pushed
the
3208-create-post-collapsible-trigger-component
branch
from
0c0a845
to
5b8d69d
Compare
There was a problem hiding this comment.
The only semantically valid child or slotted element is a single button (it still makes sense to slot it because there are many types of buttons and you'd not want to rebuild the trigger as a button). With this in mind, it would be possible to simplify the code somewhat, because clicks on buttons bubble up through the shadow dom and buttons are focusable anyways.
- No need for keydown or pointerdown, just listen for click events on the host (https://codepen.io/tuelsch/pen/zYVYgEd?editors=1111)
- No need to check if it's focusable
- Check if the slotted element is a button, otherwise warning (on componentDidLoad)
- Set all aria attributes on the slotted button
There was a problem hiding this comment.
I updated the PR to make it mandatory to use a <button> inside the trigger. Do you think we should allow the [role="button"] as well?
I also attached the click listener directly to the trigger as I need to query the element to set the ARIA attributes anyways.
Concerning slotted buttons, I am not sure this is a real use case in the end. The component itself does not have a shadow root thus in most cases both the button and the collapsible will be in the light DOM. And if we implement the post-collapsible-trigger into another component, we will most probably also add the button and the collapsible ourselves so they will both be in the shadow DOM. What do you think?
Anyways, if that's okay with you I would rather merge this PR without this feature and potentially handle slotted buttons in the accordion PR since it contains the necessary helpers already.
There was a problem hiding this comment.
I think for the moment we can go without role=button support. It's a valid use case, but pretty rare (hopefully). There are many other downsides to not using propper buttons (e.g. role buttons are not focusable by default), therefore we should not "advertise" this use case too much.
As for the slotted button, I think I missed that point in the review, or got confused by the two PRs. I'll review with that in mind again, thanks for the heads up.
packages/components/src/components/post-collapsible-trigger/post-collapsible-trigger.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/components/post-collapsible-trigger/post-collapsible-trigger.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/components/post-collapsible-trigger/post-collapsible-trigger.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/components/post-collapsible-trigger/post-collapsible-trigger.tsx
Outdated
Show resolved
Hide resolved
…st-collapsible-trigger.tsx Co-authored-by: Philipp Gfeller <[email protected]>
…s://github.com/swisspost/design-system into 3208-create-post-collapsible-trigger-component
alizedebray
force-pushed
the
3208-create-post-collapsible-trigger-component
branch
from
2723c96
to
7e0295a
Compare
…s://github.com/swisspost/design-system into 3208-create-post-collapsible-trigger-component
…s://github.com/swisspost/design-system into 3208-create-post-collapsible-trigger-component
|
No description provided.