-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
migrate: useDisclosure hook #13459
migrate: useDisclosure hook #13459
Conversation
custom replacement for Chakra-UI useDisclosure hook
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 think this hook is too complex for what we need for the new components. I've created a simpler useDisclosure
here that, I think will be a better fit to our use cases from now on. LMK what do you think, do we need more than a simple boolean flag? all the a11y attr will be handled by the radix components.
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.
This approach was primarily to match previous functionality, so the other places we use useDisclosure
could be swapped out without any additional refactoring (as in d804a56).
I'd be fine with minimizing to the example you shared since I don't believe we'll need the getButtonProps
and getDisclosureProps
helpers anymore with shadcn. In this case, I can close this PR/branch, and I can update #13456 to use your suggested useDisclosure
from 263698d inside #13449. 👍
Description
useDisclosure
hook introduced in Migrate FeedbackWidget to shadcn #13456useDisclosure
hook, updating imports to use new customsrc/hooks/useDisclosure.ts
Related Issue
shadcn migration