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(#16882) Menu's target prop fix #17145

Merged
merged 16 commits into from
Oct 14, 2024

Conversation

NabeelAyubee
Copy link
Contributor

@NabeelAyubee NabeelAyubee commented Aug 12, 2024

Closes #16882

Renamed the target prop to menuTarget for the Menu component to improve clarity.
Added the menuTarget prop to MenuButton and OverflowMenu components, allowing consumers to specify the DOM element into which the menu is portaled.

Changelog

Changed

  • The target prop has been renamed to menuTarget for improved clarity.
  • The menuTarget prop has been added to the MenuButton components to provide better control over menu rendering.
  • Added support for backward compatibility
  • Hide menuTarget prop from actions in storybook
  • Added deprecation warning for target prop

Testing / Reviewing

Testing / Reviewing

  • Run yarn test to ensure that there are no code errors.
  • Verify that the menuTarget prop correctly specifies the HTML element into which the menu is portaled. Ensure that the menu items are rendered in the target element as expected.
  • Check the Storybook integration to ensure the new prop name is reflected and functioning correctly.

@NabeelAyubee NabeelAyubee requested a review from a team as a code owner August 12, 2024 16:15
@NabeelAyubee
Copy link
Contributor Author

I have read the DCO document and I hereby sign the DCO.

Copy link

netlify bot commented Aug 12, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit f3d79a1
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/670cbe4146876a0008a2b89b
😎 Deploy Preview https://deploy-preview-17145--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Aug 12, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit f3d79a1
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/670cbe4134ce0d0008c3389e
😎 Deploy Preview https://deploy-preview-17145--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

Thanks again for taking this on! I don't think we should rename the target prop if possible. It makes the internal logic simpler, we can avoid the codemod, and consumers don't have to change any existing usages.

packages/react/src/components/Menu/Menu.tsx Outdated Show resolved Hide resolved
packages/react/src/components/MenuButton/index.tsx Outdated Show resolved Hide resolved
packages/upgrade/transforms/update-menu-target-prop.js Outdated Show resolved Hide resolved
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.

LGTM, thanks!

@NabeelAyubee
Copy link
Contributor Author

Hi @alisonjoseph & @emyarod , Can you please review the PR ? I've got one approval.

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@NabeelAyubee
Copy link
Contributor Author

Hi everyone, This PR has label status: failing CI. Can you help me know does this needs some code fixes from my end or there's some CI issue ?

@tay1orjones
Copy link
Member

@NabeelAyubee looks like the build failed. Do you see the same locally when running yarn test from the root?

@2nikhiltom 2nikhiltom requested a review from a team as a code owner October 10, 2024 09:37
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.09%. Comparing base (0015363) to head (f3d79a1).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #17145   +/-   ##
=======================================
  Coverage   79.09%   79.09%           
=======================================
  Files         407      407           
  Lines       14029    14031    +2     
  Branches     4338     4340    +2     
=======================================
+ Hits        11096    11098    +2     
  Misses       2768     2768           
  Partials      165      165           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Thanks for the assist @2nikhiltom! 🏆

@tay1orjones tay1orjones added this pull request to the merge queue Oct 14, 2024
Merged via the queue into carbon-design-system:main with commit 00ecc50 Oct 14, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
5 participants