-
Notifications
You must be signed in to change notification settings - Fork 1.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
fix(OverflowMenu): use v10 menu and export v11 menu as separate unstable component #10674
fix(OverflowMenu): use v10 menu and export v11 menu as separate unstable component #10674
Conversation
@joshblack question about the component exports: since I'm updating the |
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: ff4ea8b 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/62168c606da9430008f00085 😎 Browse the preview: https://deploy-preview-10674--carbon-react-next.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: ff4ea8b 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/62168c604a248c0008ed2122 😎 Browse the preview: https://deploy-preview-10674--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: ff4ea8b 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/62168c6085a54100088a1e61 😎 Browse the preview: https://deploy-preview-10674--carbon-elements.netlify.app |
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.
Is it possible to avoid the "V2" name?
Curious if we could keep the Menu-based OverflowMenu as unstable_OverflowMenu
and refer to it as such in both the new and old storybook under Menu
? The OverflowMenu
stories for the existing implementation in both old and new storybook would need to explicitly import the "non-next" implementation, I think?
Just trying to think of a way to keep the same naming scheme we have today in v10:
OverflowMenu
refers to the existing implementationunstable_OverflowMenu
refers to the new Menu-based implementation
yes @tay1orjones I'm definitely not married to the idea of this name. I wasn't sure what to call it, honestly. I went with v2 because I know in the past we've done that with DataTable. Once DataTableV2 replaced DataTable (OG), we changed the name to just DataTable. We can definitely raise it up to the team and see what people think is best. |
…1-overflow-menu-fix
Closes #10673
Changelog
OverflowMenu/next/OverflowMenu.js
refactor that Jan contributed into a new folderOverflowMenuV2
and exported asunstable_OverflowMenuV2
OverflowMenu/index.js
to have class wrapper since it's a class componentOverflowMenuItem/index.js
to not use the functional/next
component for now since OverflowMenu hasn't been refactored to a functional component. I didn't add the class wrapper bc it stops working with OverflowMenu (being keyboard navigable) if it's wrapped.OverflowMenu
stories which match the v10 component/stories now.unstable_OverflowMenuV2
tests with basic RTL testsTesting / Reviewing
Components/OverflowMenu
stories to make sure they look and work as v10 doesExperimental/unstable_Menu/OverflowMenuV2
to test the v11 (v2) version of OverflowMenu that was contributed by Jan. Everything should work as expected w/ just the component name changing. Can compare to existing v11 storybook example.