feat(accordion): clickable title #1033
Conversation
TylerR909
requested review from
stephenh,
bdow,
ksk5280 and
chr1sjf0x
as code owners
src/components/Accordion.tsx
Outdated
| if (setExpandedIndex) setExpandedIndex(index); | ||
| }} | ||
| > | ||
| <span css={Css.fg1.tl.$}>{title}</span> | ||
| {titleOnClick ? ( | ||
| <button |
There was a problem hiding this comment.
Suggestion: We should try to avoid nesting interactive elements. Per HTML standards, nesting of interactive elements is not permitted- it's possible, just not advisable.
Content model:
Phrasing content, but there must be no interactive content descendant and no descendant with the tabindex attribute specified.
https://html.spec.whatwg.org/multipage/form-elements.html#the-button-element
Please work with design to determine a way forward where we do not nest interactive elements. We could have buttons side by side, or just make the chevron the click target for expanding/collapsing.
I used to be pretty adamant with the old design crew about avoiding these patterns, and we've been successful so far. I'd like us to keep up with enforcing HTML standards back onto design and share the knowledge.
Fwiw, here's what GPT had to say about it:
Nesting a <button> element inside another <button> element is invalid HTML according to the HTML5 specification. This is because:
-
Semantic Incorrectness: Buttons are meant to be interactive elements, and nesting one interactive element inside another can create confusion about which element should be interactive.
-
Accessibility Issues: Screen readers and other assistive technologies may have difficulty interpreting nested buttons, leading to poor user experiences for individuals relying on these tools.
-
Browser Compatibility: Different browsers may handle nested buttons inconsistently. Some browsers may ignore the nested button, while others may render it in unexpected ways, leading to unpredictable behavior.
-
Event Handling Confusion: Nesting buttons can create complications in event handling. For instance, click events might not propagate as expected, causing issues in the functionality of the nested buttons.
To maintain valid, accessible, and predictable HTML, it's best to avoid nesting <button> elements inside each other.
bdow
dismissed
their stale review
Proposed way forward discussed on Slack. I'll be out the next couple days and do not want to block merging.
| <button {...tid.title} disabled={disabled} css={{ ...touchableStyle, ...Css.fg0.$ }} onClick={titleOnClick}> | ||
| {title} | ||
| </button> | ||
| <button {...tid.toggle} disabled={disabled} css={{ ...touchableStyle, ...Css.fg1.jcfe.$ }} onClick={toggle}> |
There was a problem hiding this comment.
I think personally I would switch the fg0/1 and have the chevon CTA be smaller, but 🤷 could really see that going either way/defer to you/design.
## [2.348.0](v2.347.2...v2.348.0) (2024-05-29) ### Features * **accordion:** clickable title ([#1033](#1033)) ([df9a8d6](df9a8d6))
|
🎉 This PR is included in version 2.348.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In DesignPackage, we want to be able to select "All these slots" by clicking on a header, like so:
This PR adds an optional
titleOnClickfunction that, when provided, slightly alter the behavior of the Accordion:fg0instead of the span'sfg1