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

Use different MenuItem props for plain text and rich text labels #6752

Closed
wants to merge 1 commit into from

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jan 7, 2025

This is an attempt to fix #6740.

The issue described there was caused when the label prop was changed from string to ComponentChildren in #6667.

The label prop is used used for both the menu content and the submenu toggle button title, and it was changed to ComponentChildren so that a new icon could be added without breaking the existing icon logic. However, for the toggle title it needs to be a string.

The solution proposed here involves adding a new optional richLabel prop, with type ComponentChildren, and make label be string.

If richLabel is not provided, label is used everywhere, as before #6667 was merged, but if it is provided, then it is used for the menu content, while label is still used for the toggle title.

Drawbacks

This solution is not perfect, as it requires leaking some internal details (the fact that label is used as a title, and therefore it needs to be a string). Furthermore, if both richLabel and label are provided for a menu item without a submenu, the label is not even used, but it's required, which is confusing.

However, this was the simplest solution, considering MenuItem's props API is relatively complex.

Alternatives

Some other alternatives I considered are:

  • Use react-render-to-string or some other way to stringify the label prop, so that we can "extract" the part we need from it for the title, while we keep it as ComponentChildren. However, this seemed hacky, and there's no way to know what's going to be the structure, so it's also a bit brittle.
    Ultimately, it was hard to remove html tags when stringifying, so I discarded this approach.
  • Change the MenuItem props so that the problem originally solved by Group type icons #6667 could be solved differently. I thought it would be ok to add an optional icon prop and keep label as string, but then I realized that prop already exists.
    Then I thought we could maybe deprecate icon and replace it with leftIcon and rightIcon, but then I realized the icon conditionally renders on the left side for top-level menu items, and on the right side when they are part of a submenu, so this would also be quite convoluted to achieve.

@acelaya acelaya requested a review from robertknight January 7, 2025 10:09
@acelaya
Copy link
Contributor Author

acelaya commented Jan 7, 2025

We have discussed a probably better approach in slack, where we keep label as ComponentChildren and provide a new prop specifically for the toggle button title, rather than trying to create it internally in MenuItem.

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

Successfully merging this pull request may close these issues.

Groups submenu expand action has tooltip of "Show action for [object Object]"
1 participant