chore(menu): move to stable #13678

janhassel · 2023-04-27T12:26:35Z

Affects:

Menu
- MenuItem
- MenuItemDivider
- MenuItemGroup
- MenuItemRadioGroup
- MenuItemSelectable
useContextMenu
ComboButton
MenuButton
OverflowMenuV2
- OverflowMenuV2 stays marked as "experimental". The component itself is moved to OverflowMenu/next and can be enabled with a feature flag (enable-v12-overflowmenu)

netlify · 2023-04-27T12:31:04Z

✅ Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`7fa11d3`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-components-react/deploys/647f57409eab1d000766a228
😎 Deploy Preview	https://deploy-preview-13678--carbon-components-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify · 2023-04-27T12:31:34Z

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	`7fa11d3`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-elements/deploys/647f5740d4cfe300082cf59f
😎 Deploy Preview	https://deploy-preview-13678--carbon-elements.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

francinelucca · 2023-04-27T19:47:19Z

Component has a label in the github repository
Currently "component: context-menu" exists
@sstrubberg could you either rename this into "component: menu" or add additional ones if we wanna split it up?
"component: menu-button"
"component: combo-button"
"component: overflow-menu-v2"

I vote for separate component tags 🙋🏽‍♀️

sstrubberg · 2023-04-27T20:28:07Z

Component has a label in the github repository
Currently "component: context-menu" exists
@sstrubberg could you either rename this into "component: menu" or add additional ones if we wanna split it up?
"component: menu-button"
"component: combo-button"
"component: overflow-menu-v2"

I vote for separate component tags 🙋🏽‍♀️

I'm indifferent until I understand the ramifications. I'd love for the rest of @carbon-design-system/developers to weigh in.

tay1orjones · 2023-05-01T20:28:28Z

It would be nice to be able to distinguish issues as relating to menu, and/or the components built atop it, menu-button, combo-button, etc.

tay1orjones · 2023-05-02T13:05:23Z

I'm not sure if we should mark OverflowMenuV2 as stable under that name. The ideal for v12 would be that the existing OverflowMenu would be replaced with the internals of OverflowMenuV2 without a name change.

Would it make more sense to instead:

make a new feature flag like v12-enable-new-overflowMenu
Toggle in overflowmenu/index.js which OverflowMenu is used depending on the value of the feature flag
Update OverflowMenuv2 to just be a wrapper using FeatureFlags

<FeatureFlags flags={v12-enable-new-overflowMenu: true}>
  <OverflowMenu {…props}
</FeatureFlags>

Place a deprecation warning on OverflowMenuV2, pointing folks to instead import OverflowMenu and wrap it in a <FeatureFlag>. This can be done at each individual usage point, globally at the root of their app, or anywhere inbetween.

This way existing usages of OverflowMenuV2 aren't impacted, but we're providing a path to a more seamless upgrade where folks won't have to change imports.

Thoughts?

A few unknowns:

How will we handle the TypeScript typings of these two components under one export?

janhassel · 2023-05-02T13:15:32Z

@tay1orjones This sounds reasonable, I can work on that. How would you structure the storybook with this change? Should there be a folder inside of the OverflowMenu stories called next which includes the three stories of OverflowMenuV2 / v12 OverflowMenu?

tay1orjones · 2023-05-05T12:44:11Z

@janhassel I'm proposing a new way to document feature flags and organize their stories over in #13382 (comment)

I made a new file /components/Tile/Tile.featureflag.stories.js to house the stories. It's title is title: 'Experimental/Feature Flags/Tile'.

You could mirror the same for now and we could add the enable-v12-overflowmenu feature flag to the feature flag overview docs story once that other PR is merged. Thoughts?

janhassel · 2023-05-19T08:47:48Z

@tay1orjones I updated the OverflowMenuV2 implementation as you described above. I was also able to already use the new WithDeprecationNotice story template 😀 https://deploy-preview-13678--carbon-components-react.netlify.app/?path=/story/experimental-unstable-overflowmenuv2--overflow-menu-v-2

Could you look over it and let me know if there's anything I missed?

tay1orjones

Percy caught this - the breadcrumb overflow icon is no longer horizontal like it should be

janhassel · 2023-05-22T13:49:05Z

@tay1orjones Hmm, this seems to come from BreadcrumbItem as it's checking whether its child is an OverflowMenu based on its displayName. I added this explicit displayName again to the forwarding component in /OverflowMenu/index.js. Let me know if this makes sense!

tay1orjones

Looks great, thanks!

tay1orjones · 2023-05-22T18:49:57Z

I just made the following labels:

We'll just reuse the existing component: overflow-menu for both versions with and without the new feature flag.

tay1orjones · 2023-05-24T15:14:35Z

Once #13840 is in, we'll also want to make the change here to remove 'aria_child_tabbable' from the denylist in playwright.config.js and config/jest-config-carbon/matchers/toHaveNoACViolations.js

We had to add it in to get that PR to pass due to the Menu's None of the descendent elements with "menuitem" role is tabbable a11y violation #12728

janhassel · 2023-05-24T16:39:33Z

@tay1orjones Just pushed the updates 👍

laurenmrice

This looks great, @janhassel ! ✨Just a couple of things:

Menu story

The caret icon in the first row of the menu needs to be vertically centered in the row with the text for all sizes. Currently, it looks to be a couple of pixels too high.

Menu button with dividers story (Safari bug)

When the menu is open it overlaps the button.

Context menu question

I don't see a story with the terminology "Context menu" specifically. Is Menu replacing this? For some reason, I was expecting to see it as its own component like Combo button, Menu button, and Overflow menu. I also can't find a way to right-click to get the menu to appear/open and was wondering if that interaction is still needed to display here or is it fine to only specify in our documentation?

janhassel · 2023-05-25T15:43:22Z

@laurenmrice

Alignment – Good catch, should be fixed now
Overlay – this is happening because your browser window isn't high enough to show the full menu right now. The Menu uses the following heuristics for positioning itself:
- Render below if there's enough space
- Otherwise render above
- If there's also not enough space, render below and stick to the bottom edge
ContextMenu story – From a technical perspective, the context menu is a regular Menu that uses the useContextMenu hook. Therefore the main story would be the Menu story and the right-click enabled one is located in the "hooks" folder in storybook: https://deploy-preview-13678--carbon-components-react.netlify.app/?path=/story/hooks-usecontextmenu--use-context-menu
- @tay1orjones Feel free to chime in if I misunderstood this part from our conversation end of last year

laurenmrice · 2023-05-31T12:37:36Z

@janhassel Okay sounds good.

francinelucca · 2023-06-01T17:13:39Z

@tay1orjones we still see the violation when removing the rule

I see the IBMa issue is still open so I don't think we can expect this false positive violation to not show up yet...

tay1orjones · 2023-06-06T15:28:30Z

@francinelucca @janhassel Ah I'm sorry, I thought the aria-owns and aria-controls updates would fix it. Looks like we can't remove that yet until the false positive is addressed, IBMa/equal-access#1402.

tay1orjones

I just re-added aria_child_tabbable to the AC denylist. This looks good to merge and should pass now 👍

janhassel added 7 commits April 27, 2023 13:25

chore(menu): update copyright banners

6a94f3f

chore(menu): remove unstable__ prefix from exports

f186335

chore(menu): update copyright banners

2053fd0

chore(menu): move to stable stories

98972ea

test(overflow-menu-v2): add vrt and avt

9214ce9

fix(menu-item): move default prop function out

d16c8f1

docs(overflow-menu-v2): add mdx

05f8bc4

janhassel requested review from a team as code owners April 27, 2023 12:26

janhassel requested review from tay1orjones and francinelucca April 27, 2023 12:26

janhassel added 3 commits April 27, 2023 15:03

fix(combo-button): sync tooltipAlignment prop naming

5c7d86b

feat(overflow-menu-v2): add props.label and props.tooltipAlignment

53d3421

fix(overflow-menu-v2): fix a11y violation with aria-owns

9c81526

janhassel mentioned this pull request May 8, 2023

feat(tiles): add disabled story and update styles #13382

Merged

janhassel added 2 commits May 19, 2023 08:23

Merge branch 'main' into 13514

31fc557

refactor(overflowmenu): expose next overflowmenu under feature flag

e4f61e9

janhassel mentioned this pull request May 22, 2023

[a11y]: OverflowMenuItem, None of the descendent elements with "menuitem" role is tabbable #12728

Open

2 tasks

tay1orjones requested changes May 22, 2023

View reviewed changes

fix(overflowmenu): add back explicit displayName

b0bf3ad

francinelucca added the status: one more review 👀 label May 22, 2023

tay1orjones approved these changes May 22, 2023

View reviewed changes

tay1orjones added status: visual review 🔍 and removed status: one more review 👀 labels May 22, 2023

tay1orjones requested a review from laurenmrice May 22, 2023 18:51

francinelucca mentioned this pull request May 23, 2023

fix: add aria-controls to buttons controlling menus #13823

Merged

tay1orjones mentioned this pull request May 24, 2023

chore(achecker): update accessibility-checker ruleset #13840

Merged

janhassel and others added 3 commits May 24, 2023 18:34

fix: add aria-controls, move aria-owns to parent container

5b1e2f7

Merge branch 'main' into 13514

3200614

Merge branch 'main' into 13514

b2ea7bc

laurenmrice requested changes May 25, 2023

View reviewed changes

fix(menu): vertically center-align nesting caret

b33919f

Merge branch 'main' into 13514

2cadfbc

laurenmrice approved these changes May 31, 2023

View reviewed changes

ci: remove aria_child_tabbable from testing denylists

196f0dd

Merge branch 'main' into 13514

0a84241

test: re-add aria_child_tabbable to AC denylist

e2a06cf

tay1orjones approved these changes Jun 6, 2023

View reviewed changes

Merge branch 'main' into 13514

e8f76b4

tay1orjones added the status: ready to merge 🎉 label Jun 6, 2023

Merge branch 'main' into 13514

7fa11d3

kodiakhq bot merged commit 7a5a989 into carbon-design-system:main Jun 6, 2023

janhassel deleted the 13514 branch June 7, 2023 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(menu): move to stable #13678

chore(menu): move to stable #13678

janhassel commented Apr 27, 2023 •

edited by tay1orjones

Loading

netlify bot commented Apr 27, 2023 •

edited

Loading

netlify bot commented Apr 27, 2023 •

edited

Loading

francinelucca commented Apr 27, 2023

sstrubberg commented Apr 27, 2023

tay1orjones commented May 1, 2023

tay1orjones commented May 2, 2023

janhassel commented May 2, 2023

tay1orjones commented May 5, 2023 •

edited

Loading

janhassel commented May 19, 2023

tay1orjones left a comment

janhassel commented May 22, 2023

tay1orjones left a comment

tay1orjones commented May 22, 2023

tay1orjones commented May 24, 2023

janhassel commented May 24, 2023

laurenmrice left a comment •

edited

Loading

janhassel commented May 25, 2023

laurenmrice commented May 31, 2023

francinelucca commented Jun 1, 2023

tay1orjones commented Jun 6, 2023

tay1orjones left a comment

chore(menu): move to stable #13678

chore(menu): move to stable #13678

Conversation

janhassel commented Apr 27, 2023 • edited by tay1orjones Loading

netlify bot commented Apr 27, 2023 • edited Loading

✅ Deploy Preview for carbon-components-react ready!

netlify bot commented Apr 27, 2023 • edited Loading

✅ Deploy Preview for carbon-elements ready!

francinelucca commented Apr 27, 2023

sstrubberg commented Apr 27, 2023

tay1orjones commented May 1, 2023

tay1orjones commented May 2, 2023

janhassel commented May 2, 2023

tay1orjones commented May 5, 2023 • edited Loading

janhassel commented May 19, 2023

tay1orjones left a comment

Choose a reason for hiding this comment

janhassel commented May 22, 2023

tay1orjones left a comment

Choose a reason for hiding this comment

tay1orjones commented May 22, 2023

tay1orjones commented May 24, 2023

janhassel commented May 24, 2023

laurenmrice left a comment • edited Loading

Choose a reason for hiding this comment

janhassel commented May 25, 2023

laurenmrice commented May 31, 2023

francinelucca commented Jun 1, 2023

tay1orjones commented Jun 6, 2023

tay1orjones left a comment

Choose a reason for hiding this comment

janhassel commented Apr 27, 2023 •

edited by tay1orjones

Loading

netlify bot commented Apr 27, 2023 •

edited

Loading

netlify bot commented Apr 27, 2023 •

edited

Loading

tay1orjones commented May 5, 2023 •

edited

Loading

laurenmrice left a comment •

edited

Loading