-
-
Notifications
You must be signed in to change notification settings - Fork 276
[accordion][collapsible] Add data-hidden attribute to collapsible and accordion when they are not open #3063
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
base: master
Are you sure you want to change the base?
Conversation
commit: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Maybe Lines 89 to 91 in e9f98ed
|
When using Tailwind, passing a display class (like block or flex) to the collapsible body will override the built-in hidden attribute, since Tailwind’s utility classes take precedence in the final CSS. By adding a data-hidden attribute, we can increase CSS specificity and ensure the “hidden” state is correctly applied, even when other display utilities are present. |
|
You can technically still style based on the hidden attribute in tailwind (and that's what I'm doing now), but it's a lot less intuitive: vs. |
👍 makes sense ~ we have |
| hidden: !open, | ||
| transitionStatus, | ||
| }), | ||
| [state, transitionStatus], | ||
| [state, transitionStatus, open], |
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.
open is already in the ...state, we don't need to modify the state to add a new data attr based on it, instead modify the state<>data attr mapping here:
base-ui/packages/react/src/utils/collapsibleOpenStateMapping.ts
Lines 26 to 35 in e9f98ed
| export const collapsibleOpenStateMapping = { | |
| open(value) { | |
| if (value) { | |
| return PANEL_OPEN_HOOK; | |
| } | |
| return PANEL_CLOSED_HOOK; | |
| }, | |
| } satisfies StateAttributesMapping<{ | |
| open: boolean; | |
| }>; |
|
@mj12albert I updated the PR to update the data attribute states in the common functions. There are some different patterns in the repo for assembling these state attributes. I tried to match each collapsible/accordion to other parts of their "component" (e.g. the triggers and the roots), but it still felt a little awkward to merge these with the "parent" data-attributes in this way - let me know what you think! |
676cd67 to
8bc13d3
Compare
|
|
||
| const stateAttributesMapping: StateAttributesMapping<CollapsiblePanel.State> = { | ||
| ...panelOpenStateMapping, | ||
| ...transitionStatusMapping, |
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.
The transitionStatusMapping can be added directly to collapsibleOpenStateMapping since it's the same for both panel components
mj12albert
left a comment
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.
@BrandonGoren I just remembered that internally hidden does not 100% correspond to open:
base-ui/packages/react/src/collapsible/panel/useCollapsiblePanel.ts
Lines 50 to 56 in 2b4e9c2
| const hidden = React.useMemo(() => { | |
| if (animationTypeRef.current === 'css-animation') { | |
| return !visible; | |
| } | |
| return !open && !mounted; | |
| }, [open, mounted, visible, animationTypeRef]); |
Instead I think props.hidden that useCollapsiblePanel returns needs to be added to the state in both panel components
Adds a data-hidden attribute to collapsible and accordion when they are not open to match the behavior in tabs (which sets this attribute).
Attached is a screenshot from an accordion component where data-hidden is now applied!