Skip to content
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

Merged
merged 16 commits into from
Feb 23, 2022

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Feb 9, 2022

Closes #10673

Changelog

  • Moved OverflowMenu/next/OverflowMenu.js refactor that Jan contributed into a new folder OverflowMenuV2 and exported as unstable_OverflowMenuV2
  • Updated OverflowMenu/index.js to have class wrapper since it's a class component
  • Updated OverflowMenuItem/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.
  • Added some v11 styles that were breaking v11 breadcrumb overflow menu
  • Added v11 OverflowMenu stories which match the v10 component/stories now.
  • Updated the inline notification warning on the v11 unstable menu stories
  • Updated unstable_OverflowMenuV2 tests with basic RTL tests

Testing / Reviewing

  • make sure tests are passing
  • review v11 storybook deploy preview
    • look at Components/OverflowMenu stories to make sure they look and work as v10 does
    • look at Experimental/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.

@jnm2377 jnm2377 requested a review from a team as a code owner February 9, 2022 19:16
@jnm2377
Copy link
Contributor Author

jnm2377 commented Feb 9, 2022

@joshblack question about the component exports: since I'm updating the react package index.js to export Jan's menu as a separate unstable component, I didn't include that in the carbon-react package index.js exports bc it's exporting from carbon-components-react and I thought we would need to release first to have the new unstable export available. Is that a correct assumption?

@netlify
Copy link

netlify bot commented Feb 9, 2022

✔️ 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

@netlify
Copy link

netlify bot commented Feb 9, 2022

✔️ 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

@netlify
Copy link

netlify bot commented Feb 9, 2022

✔️ 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

Copy link
Member

@tay1orjones tay1orjones left a 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 implementation
  • unstable_OverflowMenu refers to the new Menu-based implementation

@jnm2377
Copy link
Contributor Author

jnm2377 commented Feb 22, 2022

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 implementation
  • unstable_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide on direction for OverflowMenuItem vs MenuItem
3 participants