[accordion] New Accordion components and hooks #577
Conversation
Netlify deploy preview |
36e29e3
to
881476c
Compare
84b0482
to
e16cad0
Compare
bb181bf
to
c735efb
Compare
b5f818e
to
6969cd2
Compare
6969cd2
to
91333ff
Compare
91333ff
to
85062eb
Compare
| customStyleHookMapping: { | ||
| disabled: (isDisabled) => { | ||
| if (isDisabled) { | ||
| return { 'data-disabled': '' }; | ||
| } | ||
| return null; | ||
| }, | ||
| value: () => null, | ||
| }, |
There was a problem hiding this comment.
Should be placed outside the component. I don't think data-disabled needs a custom style hook as this should work by default?
There was a problem hiding this comment.
@atomiks Moved - by default disabled becomes data-disabled="true" instead of just data-disabled which seems a bit better, maybe the getStyleHookProps util could be updated to handle this universally?
There was a problem hiding this comment.
I'm ok with just data-disabled and whatever we choose, we should apply to all components. So getStyleHookProps should handle it.
There was a problem hiding this comment.
2c8327e
Since we already don't render any data attr when the value is false, I think we could change all data-attr='true' to just data-attr (e.g. data-readonly, data-required...)
| When uncontrolled, use the `defaultValue` prop to set the initial state of the accordion: | ||
|
|
||
| ```tsx | ||
| <Accordion.Root defaultValue={[0]}> |
There was a problem hiding this comment.
I wonder if openPanels wouldn't make more sense than value (and now that I think of it, I'd rename Tabs's value as well).
There was a problem hiding this comment.
I agree the API for Tabs and Accordion should be as similar as possible, wonder what @vladmoroz and @colmtuite think?
The API would be openPanels/defaultOpenPanels? and onOpenPanelChange?
There was a problem hiding this comment.
@michaldudak what would value become on the item itself? E.g. here <Accordion.Item value="1">
There was a problem hiding this comment.
value on items still makes sense to me.
There was a problem hiding this comment.
If items have value, I think the connection gets lost then if the root uses anything else other than value
There was a problem hiding this comment.
But it's not the same thing. A panel has a value (or really, an identifier, so we could consider using the id here as well), and the Accordion controls which panels are open. Using the same prop for both is counterintuitive for me.
React Aria's DisclosureGroup uses expandedKeys and id (a bit weird as well, as "key" != "id")
Ariakit's Tab uses selectedId and id.
There was a problem hiding this comment.
For using id, I worry it's not a good idea to conflate the "panel value" with the id attribute 🤔
but since this is equally relevant to Tabs let's discuss this in the next call to unblock this PR 🙏
There was a problem hiding this comment.
I'm not entirely convinced it's the best idea either, especially that on Tabs you can use non-string values as well, which won't be possible with id. But then, is it a real use case?
There was a problem hiding this comment.
We discussed this and decided to stick with value/onValueChange for general consistency, all the other options we could come up with are a bit clumsy by comparison
2c8327e
to
e1134df
Compare
5dc3aac
to
e7de337
Compare
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michał Dudak <michal@mui.com> Co-authored-by: atomiks <cc.glows@gmail.com>
Closes #25
Closes #627
Closes #702
Closes mui/material-ui#36297
Builds on top of:
RadioGroupcomponent #487Preview
👉 https://deploy-preview-577--base-ui.netlify.app/components/react-accordion
Summary
Accordion is built using Collapsible, some changes were made to
Collapsiblewhile working on this:useCollapsibleTrigger(andCollapsible/AccordionTrigger) now usesuseButtonCollapsible.Contentis now renamed toCollapsible.Panelwindow.getComputedStyleto an absolute minimum, merged the internal states into one[data-open]on the panel, and[data-panel-open]on the triggerAccordion features
orientation: 'vertical' | 'horizontal': sets a data attr and navigates focus between triggers with ArrowRight/Left instead of ArrowDown/UpCollapsible.Contentnow exposes the--collapsible-content-widthvar to support horizontal orientationdirection: 'ltr' | 'rtl': sets thedirattr like other components and reverses ArrowRight & ArrowLeft whenorientation === 'horizontal'openMultiple: boolean: defaulttrue, controls whether multiple sections can be open at the same time, not expected to change during the lifetime of the componentdisabledcan be set on theRoot,Section, orTriggerhidden="until-found"can be set on theRootor aPanelExtra demos
hidden="until-found"https://deploy-preview-577--base-ui.netlify.app/experiments/accordion-animations