-
-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Accordion] New Accordion components and hooks #577
base: master
Are you sure you want to change the base?
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
f9e05b1
to
fc907ba
Compare
docs/data/components/accordion/UnstyledAccordionIntroduction.tsx
Outdated
Show resolved
Hide resolved
describeConformance(<Accordion.Heading />, () => ({ | ||
inheritComponent: 'h3', | ||
render: (node) => { | ||
const { container, ...other } = render( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this pattern in a couple of your components. Why not just return render(...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just copy-pasta 🙈
packages/mui-base/src/Accordion/Heading/AccordionHeading.test.tsx
Outdated
Show resolved
Hide resolved
93364ca
to
9f531a6
Compare
Fix uncontrolled mode Fix broken links
9f531a6
to
3c687fa
Compare
a5475e4
to
28978bb
Compare
const { ownerState } = useAccordionItemContext(); | ||
|
||
const { renderElement } = useComponentRenderer({ | ||
render: render ?? 'h3', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h3
seems arbitrary here. I wonder if a div won't be a safer option.
props: AccordionHeader.Props, | ||
forwardedRef: React.ForwardedRef<HTMLHeadingElement>, | ||
) { | ||
const { render, className, ...otherProps } = props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency
const { render, className, ...otherProps } = props; | |
const { render, className, ...other } = props; |
export { AccordionHeader }; | ||
|
||
export namespace AccordionHeader { | ||
export interface Props extends BaseUIComponentProps<'h3', AccordionItem.OwnerState> {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export { AccordionHeader }; | |
export namespace AccordionHeader { | |
export interface Props extends BaseUIComponentProps<'h3', AccordionItem.OwnerState> {} | |
} | |
namespace AccordionHeader { | |
export interface Props extends BaseUIComponentProps<'h3', AccordionItem.OwnerState> {} | |
} | |
export { AccordionHeader }; |
- other files
// import { expect } from 'chai'; | ||
// import { spy } from 'sinon'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// import { expect } from 'chai'; | |
// import { spy } from 'sinon'; |
const { render } = createRenderer(); | ||
|
||
describeConformance(<Accordion.Header />, () => ({ | ||
inheritComponent: 'h3', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inheritComponent
is not used anymore. You can remove it from all the tests.
onOpenChange: onOpenChangeProp, | ||
render, | ||
value: valueProp, | ||
...otherProps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...otherProps | |
...other |
...otherProps | ||
} = props; | ||
|
||
const sectionRef = React.useRef<HTMLElement>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this ref is unused
if (onOpenChangeProp) { | ||
onOpenChangeProp(nextOpen); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (onOpenChangeProp) { | |
onOpenChangeProp(nextOpen); | |
} | |
onOpenChangeProp?.(nextOpen); |
// import { expect } from 'chai'; | ||
// import { spy } from 'sinon'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// import { expect } from 'chai'; | |
// import { spy } from 'sinon'; |
@@ -0,0 +1,10 @@ | |||
export { AccordionRoot as Root } from './Root/AccordionRoot'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not export hooks and contexts. We may revisit them in the future, but for now we decided to export just the components
Closes #25
Builds on top of:
RadioGroup
component #487Preview
👉 https://deploy-preview-577--base-ui.netlify.app/components/react-accordion
Summary
orientation: 'vertical' | 'horizontal'
: sets a data attr and navigates focus between triggers with ArrowRight/Left instead of ArrowDown/UpCollapsible.Content
now exposes the--collapsible-content-width
var to support horizontal orientationdirection: 'ltr' | 'rtl'
: sets thedir
attr 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 componentdisabled
can be set on theRoot
,Section
, orTrigger
hidden="until-found"
can be set on theRoot
or aPanel
useCollapsibleTrigger
(andCollapsible/AccordionTrigger
) now usesuseButton
Extra demos
hidden="until-found"
https://deploy-preview-577--base-ui.netlify.app/experiments/accordion-animations