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

feat(menu): deprecate props.mode / always support full capabilities + icons #18153

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

janhassel
Copy link
Member

Closes #18148, #15875, #17725
Related #18084, https://ibm-analytics.slack.com/archives/C0M053VPT/p1715933314490569

Deprecates props.mode for Menu and always support the full list of capabilities:

  • Selectable items (single and multi select)
  • Nested menus
  • Icons

The original concern of having a checkmark icon and a custom icon on the same visual level is being addressed by placing them in separate columns. The original concern was that users wouldn't be able to distinguish quickly what's a decorative icon and what is a status icon. The design was discussed with @thyhmdo and @ariellalgilmore.

Changelog

Changed

  • Updated styles to support a status icon column in menu items
  • Moved checkmark icon logic from MenuItemSelectable and MenuItemRadioGroup into MenuItem based on aria-checked

Removed

  • Removed all usage of props.mode
  • Removed mode logic from menu context
  • Removed warnings for invalid mode and feature pairings

image

@thyhmdo Side note: even if technically selectable items inside of menu and combo buttons would be allowed with this approach, we should probably add some details in the design guidance discouraging this. Also, here's our related discussion: https://ibm-analytics.slack.com/archives/C07HHD0CNR4/p1725459515044009

General side note: this adds support for icons for MenuItemSelectable. We should probably add support for MenuItemRadioGroup as well but it's not compatible with the current structure. My long-term thought is to replace the props.items with something like a MenuItemRadioOption component that should be used as children of MenuItemRadioGroup. But that's probably better discussed in a separate issue.


Opening as draft as an issue occurs with the nested MenuButton. For some reason, the overflow-y: auto causes the nested menu to be clipped in MenuButton but not Menu. I'll need to investigate further.

Copy link

netlify bot commented Nov 25, 2024

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit b041efe
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/6784d4a69c40c50008d22b85
😎 Deploy Preview https://deploy-preview-18153--v11-carbon-web-components.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 Nov 25, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit b041efe
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6784d4a6d56e31000815b061
😎 Deploy Preview https://deploy-preview-18153--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

netlify bot commented Nov 25, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b041efe
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6784d4a62c32f100082920f0
😎 Deploy Preview https://deploy-preview-18153--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.

@janhassel
Copy link
Member Author

@tay1orjones It seems that using transform on the root menu causes the child menu to base it's boundaries on the parent for some reason. I couldn't find any related spec detailing this and I haven't seen this before (I think).

As a test, I wrote a janky middleware for floating-ui that adds the intended transform to top and left and it resolves the problem. Do you have an idea how to proceed here?

floatingSize({
  apply({ elements }) {
    const { transform, top: topPx, left: leftPx } = elements.floating.style;

    if (transform) {
      const [_, xPx, yPx] = transform.match(/translate\((.+), (.+)\)/);
      const values = [topPx, yPx, leftPx, xPx].map((px) => Number(px.replace('px', '')));

      Object.assign(elements.floating.style, {
        top: `${values[0] + values[1]}px`,
        left: `${values[2] + values[3]}px`,
        transform: 'unset',
        willChange: 'unset',
      });
    }
  }
})

With middleware:
image

Without middleware (Safari):
image

You can see the box-shadow of the nested menu on the right side of "Delete", showing that it renders but is clipped.

In Edge, a scroll is added to the parent (without middleware):
image

@tay1orjones
Copy link
Member

tay1orjones commented Dec 10, 2024

@janhassel I dug around a bit and found this floating-ui/floating-ui#2858

It might be worth trying to set the boundary to [] in one or more middleware's instead of the default clippingAncestors. Might be why you're seeing that specifically with nesting.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.14%. Comparing base (e973f28) to head (b041efe).

Files with missing lines Patch % Lines
packages/react/src/components/MenuButton/index.tsx 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18153      +/-   ##
==========================================
- Coverage   84.17%   84.14%   -0.03%     
==========================================
  Files         408      408              
  Lines       14435    14429       -6     
  Branches     4690     4655      -35     
==========================================
- Hits        12150    12142       -8     
- Misses       2121     2122       +1     
- Partials      164      165       +1     

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

@janhassel
Copy link
Member Author

@tay1orjones Thank you for the link! After further research I found the culprit: when an element is using transform it creates a new containing block which all (also fixed-positioned) descendants reference.

For elements whose layout is governed by the CSS box model, any value other than none for the transform property also causes the element to establish a containing block for all descendants. Its padding box will be used to layout for all of its absolute-position descendants, fixed-position descendants, and descendant fixed background attachments.

https://www.w3.org/TR/2019/CR-css-transforms-1-20190214/#current-transformation-matrix-computation

This caused the submenu to fall into the overflow clipping of the parent menu which we need in order to offer vertical scroll. I updated the configuration for useFloating to use absolute positioning instead of transform. All demo stories seem to work as intended now.

@janhassel janhassel marked this pull request as ready for review January 13, 2025 08:54
@janhassel janhassel requested review from a team as code owners January 13, 2025 08:54
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.

[Feature Request]: v12 overflowMenu support for renderIcon prop on MenuItem. and checkbox
2 participants