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.

Sign up for GitHub

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

Jump to bottom

[Bug]: MenuButton and OverflowMenu cannot configure the Menu's target prop #16882

Closed
2 tasks done
tay1orjones opened this issue Jun 27, 2024 · 2 comments · Fixed by #17145
Closed
2 tasks done

[Bug]: MenuButton and OverflowMenu cannot configure the Menu's target prop #16882

tay1orjones opened this issue Jun 27, 2024 · 2 comments · Fixed by #17145
Labels
afrohacks See https://ibm.biz/afrohacks-hackathon component: menu-button component: overflow-menu good first issue 👋 Used by GitHub to elevate contribution opportunities hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. role: dev 🤖 severity: 3 https://ibm.biz/carbon-severity status: help wanted 👐 type: bug 🐛

Comments

@tay1orjones
Copy link
Member

tay1orjones commented Jun 27, 2024 •
edited
Loading

Package

@carbon/react

Browser

Chrome

Package version

11.60.3

React version

18

Description

Surfaced in slack
MenuButton and OverflowMenu use Menu. Menu has a target prop for controlling what DOM element the ul is portaled into. This prop is not exposed through these components leaving consumers with a lack of control of where the menu is being rendered.

This needs to be exposed. I'd like to avoid prop drilling but in this case I don't think there's a viable alternative. It might be beneficial to call it menuTarget on these components.

Reproduction/example

react.carbondesignsystem.com

Steps to reproduce

Just take a look at the source, there's no target prop and ...rest is spread onto the containing div.

Suggested Severity

Severity 3 = User can complete task, and/or has a workaround within the user experience of a given component.

Application/PAL

No response

Code of Conduct
@tay1orjones tay1orjones moved this to ⏱ Backlog in Design System Jun 27, 2024
@tay1orjones tay1orjones added status: help wanted 👐 good first issue 👋 Used by GitHub to elevate contribution opportunities hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. afrohacks See https://ibm.biz/afrohacks-hackathon labels Jun 27, 2024
@tay1orjones tay1orjones mentioned this issue Jun 27, 2024
Open
1 task
@NabeelAyubee NabeelAyubee mentioned this issue Aug 8, 2024
Closed
@NabeelAyubee
Copy link
Contributor

NabeelAyubee commented Aug 8, 2024 •
edited
Loading

Hi, I have tried to go through the issue. I have renamed the target prop as per the description and expose menuTarget prop from MenuButton component. Also I have tried to test in storybook, that the menuTarget prop correctly specifies the HTML element into which the menu is portaled. Menu items are rendered in the target element as expected.

Please review my PR and help me if I missed anything.

PS: This is my first time. Please help me, if I missed something.

NabeelAyubee added a commit to NabeelAyubee/carbon that referenced this issue Aug 12, 2024
@NabeelAyubee
fix(menu): carbon-design-system#16882 Menu's target prop fix
a105f85
@NabeelAyubee NabeelAyubee mentioned this issue Aug 12, 2024
Merged
@NabeelAyubee
Copy link
Contributor

NabeelAyubee commented Aug 12, 2024 •
edited
Loading

Hi, had to close my previous PR due to some unintended merges.

I have tried to resolve the issue and also resolved previous PR comments suggested by @tay1orjones.

  • I have added menuTarget prop from MenuButton component also added backward compatiblity for target prop and added deprecation warning for target prop.
  • Added menuTarget prop for menuButton and Overflow feature flag compoent.
  • Disable menuTarget prop from storybook actions for menu, menuButton and overflowMenu
  • Also I have tried to test in storybook, that the menuTarget prop correctly specifies the HTML element into which the menu is portaled. Menu items are rendered in the target element as expected.

Please review my PR and help me if I missed anything.
Link: #17145

NabeelAyubee added a commit to NabeelAyubee/carbon that referenced this issue Aug 13, 2024
@NabeelAyubee
fix(menu pr resolve): carbon-design-system#16882 Menu's target prop fix
31a1807
@2nikhiltom 2nikhiltom mentioned this issue Oct 9, 2024
Closed
2 tasks
github-merge-queue bot pushed a commit that referenced this issue Oct 14, 2024
@NabeelAyubee @emyarod
@2nikhiltom @tay1orjones @kennylam
fix(#16882) Menu's target prop fix (#17145)
00ecc50 
* fix(menu): #16882 Menu's target prop fix

* fix(menu pr resolve): #16882 Menu's target prop fix

* fix: ts error fixed, docs,storybook eg & snapshot updated

---------

Co-authored-by: andrew <[email protected]>
Co-authored-by: Nikhil Tomar <[email protected]>
Co-authored-by: Taylor Jones <[email protected]>
Co-authored-by: kennylam <[email protected]>
Co-authored-by: Nikhil Tomar <[email protected]>
@github-project-automation github-project-automation bot moved this from ⏱ Backlog to ✅ Done in Design System Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
afrohacks See https://ibm.biz/afrohacks-hackathon component: menu-button component: overflow-menu good first issue 👋 Used by GitHub to elevate contribution opportunities hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. role: dev 🤖 severity: 3 https://ibm.biz/carbon-severity status: help wanted 👐 type: bug 🐛
Projects
Design System
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(#16882) Menu's target prop fix NabeelAyubee/carbon
fix: 16882 Menu's target prop fix
3 participants
@tay1orjones @sstrubberg @NabeelAyubee