feat: Update Accordion expansion behavior#3262
Conversation
- When `multiselectable` is not enabled, only expand the last item with
`expanded: true` on mount.
- When new items are added to the accordion:
- If `multiselectable` is enabled, maintain existing expansions and
respect `expanded: true` value of new items.
- If `multiselectable` is not enabled, either maintain the existing
expansion if no new items have `expanded: true` or collapse the
existing expansion and expand the last new item with `expanded:
true`.
- Add and update tests for the updated behavior.
|
@brandonlenz - Is this pull request something you all would be willing to consider? I'm mostly interested in respecting the |
- Switch dictionary to Map to address eslint error `security/detect-object-injection`
- Replace `.toArray()` with `Array.from()`
- Fix Node.js 20 compatibility
- Rename variable for consistency
At a first glance, this seems reasonable to me, but we ultimately just have to make sure our component behaves the same way as the base USDWS version of the component. I'm going to take a look today and verify against the USWDS implementation. My gut says we should also log an error when |
| const newExpansions = buildExpansions(newItems, multiselectable) | ||
| const newEntries = Array.from(newExpansions.entries()) | ||
| if (!multiselectable && newEntries.some(([, value]) => value)) { | ||
| updatedExpansions.forEach((val, key, map) => map.set(key, false)) |
There was a problem hiding this comment.
| updatedExpansions.forEach((val, key, map) => map.set(key, false)) | |
| updatedExpansions.forEach((_val, key, map) => map.set(key, false)) |
| newOpenItems.splice(0, newOpenItems.length) | ||
| newOpenItems.push(itemId) | ||
| if (!multiselectable) { | ||
| updatedExpansions.forEach((val, key, map) => map.set(key, false)) |
There was a problem hiding this comment.
| updatedExpansions.forEach((val, key, map) => map.set(key, false)) | |
| updatedExpansions.forEach((_val, key, map) => map.set(key, false)) |
|
Thanks for taking a look at this PR @brandonlenz . In the time since I posted this, I had to move to our own implementation of the accordion because refs associated with elements inside accordion items become mis-associated when array items are removed (I believe this is due to the component using array index for the |
Understood, I think we're on the same page actually! I'm toying with a simplification of the accordion because my gut reaction was that a useEffect might be overkill for this. Also, to follow up on my earlier note about USWDS implementation, I believe this matches. The current USWDS looks as though it loops through each accordion item and targets the open state based on the aria-expanded property: If the accordion is not does not support multiselect, the last one would "win" and be open: I'll follow up later today, hopefully on the simplification. If you already have something for that, feel free to push it up to your PR though. Having the tests in place on this branch already is very helpful as I poke around refactoring but making sure the behavior you need still works. |
Sounds good, thanks. Our component is sufficiently different that I don't have changes prepared for immediate post; I'd need to work on a changeset specific to this project. Happy to do so or to let you take the reigns. |
|
@brandonlenz - following up on the useEffect usage and the key issue:
|
brandonlenz
left a comment
brandonlenz
left a comment
There was a problem hiding this comment.
A couple of minor suggestions, not blocking.
I'll let you weigh in on what you'd like to do before merging.
- Minor optimizations
Use initializer function for initial expansions state
2 participants
multiselectableis not enabled, only expand the last item withexpanded: trueon mount.multiselectableis enabled, maintain existing expansions and respectexpanded: truevalue of new items.multiselectableis not enabled, either maintain the existing expansion if no new items haveexpanded: trueor collapse the existing expansion and expand the last new item withexpanded: true.Dev notes
Opted for
Maphere since the expansion dictionary keys come from users (consumers of this package, not typically end-users, though that is possible). This satisfies lint checks forsecurity/detect-object-injection.