carbon-design-system / carbon

A design system built by IBM
https://www.carbondesignsystem.com
Apache License 2.0
7.84k stars 1.81k forks source link

[Bug]: Menu button nested menu items warning #15875

Open misiekhardcore opened 7 months ago

misiekhardcore commented 7 months ago

Package

@carbon/react

Browser

Chrome

Package version

v1.51.1

React version

17.0.2

Description

When using the MenuButton with nested MenuItems we get the warning in the console: Screenshot from 2024-03-04 08-51-08 I can see that in MenuButton implementation the inner Menus prop mode is set to basic, so this is the reason I assume, but what is it for a purpose or is it a bug?

Reproduction/example

https://stackblitz.com/edit/github-rztleo?file=src%2FApp.jsx

Steps to reproduce

Suggested Severity

None

Application/PAL

No response

Code of Conduct

tay1orjones commented 7 months ago

@misiekhardcore Hey, thanks for reporting this. The stackblitz link you posted isn't working for me.

misiekhardcore commented 7 months ago

hey @tay1orjones I think it was set to private. Please let me know if it works now

guidari commented 7 months ago

Hey @misiekhardcore That is intentional and not a bug. Check this description for more information: https://github.com/carbon-design-system/carbon/blob/00dc60f210edeaa83bebbb96dbbda50285941cf5/packages/react/src/components/Menu/Menu.tsx#L463-L470

misiekhardcore commented 7 months ago

Hey @guidari I've seen that description but it does not really explain why in MenuButton this is set to basic (the description says it will be determined automatically but in this case it is not)

janhassel commented 7 months ago

Hey Michał, the behavior is working as expected. Both MenuButtons and ComboButtons are not intended to be used for complex scenarios like nested or selectable items. This is reserved for OverflowMenu (only with enable-v12-overflowmenu flag) and the core Menu (used by useContextMenu).

@thyhmdo Did we ever clarify that to the design guidance? I can't find it right now.

misiekhardcore commented 7 months ago

@janhassel I see, thanks for the clarification. For me the OverflowMenu is not a solution because I need it to be primary and with text. Something like this Screenshot from 2024-03-11 15-23-32 I would be also interested with the design guide explanation for disabling nested menu for the MenuButton

thyhmdo commented 6 months ago

@misiekhardcore We didn't clarify this in our guidance, but we will add this update for nesting options in MenuButton after a discussion with other product teams. Meanwhile, another solution is as Jan's suggestion here without violating the code. It can use the divider to separate categories.

image

misiekhardcore commented 6 months ago

@thyhmdo hey, thanks for the suggestion. Unfortunately, I think this won't work in our case because if we merge all the things from all the sub-menus into just one list divided by separators we are encountering two issues:

  1. the list is going to be rather long and unhandy in my opinion
  2. sub-menus had the advantage of "labeling" (e.g. "Report" sub-menu tells the user that all the items in this sub-menu are report related) all the related items and the separator line does not really provide that
thyhmdo commented 4 months ago

thanks @misiekhardcore We will do another research and clarify our guidance on this issue

tay1orjones commented 3 months ago

FWIW It sounds like this used to work in @carbon/react@1.48.1

I'm also not sure this is a bug, we might want to classify this moreso as an enhancement. If it does work in an older version, we never wrote tests for it or included it in the storybook as an example.