Update defaults of allowMultipleExpanded and allowZeroExpanded to true#366
Update defaults of allowMultipleExpanded and allowZeroExpanded to true#366liamjohnston wants to merge 4 commits intomainfrom
Conversation
wilsonespina
left a comment
wilsonespina
left a comment
There was a problem hiding this comment.
The new default functionality works well, just a couple of nitpicks with the tests.
🕺
integration/src/index.tsx
Outdated
| ReactDOM.render( | ||
| <div id="classic-accordion"> | ||
| <Accordion> | ||
| <Accordion allowMultipleExpanded={false} allowZeroExpanded={false}> |
There was a problem hiding this comment.
Shouldn't the integration tests stick to testing the default Accordian behaviour?
There was a problem hiding this comment.
It was either update the tests or update the accordion, and I chose the lazy option 😬
Fair call, I will take a look.
There was a problem hiding this comment.
Hmm, if I change it to run against the default settings, the integration test fails (as I expected).
However, the failing integration test in question is written assuming the old default behaviour. Whereas the other tests don't – they just test based on the DOM they see (e.g. if an accordion item is aria-expanded, the panel should be visible). The integration tests run in a browser (with puppeteer?).
The failing test opens the first panel, then expects that it can't be collapsed (because it's the only one open, and allowZeroExpanded:false was the default behaviour).
RAA doesn't add any DOM cues regarding allowMultipleExpanded or allowMultipleExpanded, so we can't really test against them.
I think the best course of action here is to remove that particular test. We already have an integration test that opening a panel works.
The alternative is adding more DOM guff like data-disallow-multiple-expanded/data-disallow-zero-expanded, but that feels like a too-big change to me.
fix typo Co-authored-by: Wilson Espina <wilson.espina@gmail.com>
…have changed). - Remove integration test that was testing the old default, rather than the correct behaviour.
…springload/react-accessible-accordion into feature/change-default-settings
3 participants
An approach to improving default UX.
Related discussions:
I had to bypass commit hooks to get here. I didn't have luck correctly fixing an issue running
yarn typecheck. Will come back to that later.There are still
github pagesthings to manage for this too, plus actually publishing the new version to npm. But that's only if/when we're happy with it.