fix(Options menu): added support for groups and titles by nicolethoen · Pull Request #3360 · patternfly/patternfly-react · GitHub

Addresses #3338

Follow up to core PR #2403

PatternFly-React preview: https://patternfly-react-pr-3360.surge.sh

Codecov Report

Merging #3360 into master will decrease coverage by 0.01%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master    #3360      +/-   ##
==========================================
- Coverage   67.49%   67.47%   -0.02%     
==========================================
  Files         897      897              
  Lines       25158    25166       +8     
  Branches     2181     2187       +6     
==========================================
+ Hits        16980    16981       +1     
- Misses       7162     7163       +1     
- Partials     1016     1022       +6

Flag	Coverage Δ
#misc	95.45% <ø> (ø)	⬆️
#patternfly3	69.28% <ø> (ø)	⬆️
#patternfly4	64.91% <63.63%> (-0.04%)	⬇️

Impacted Files	Coverage Δ
...rc/components/OptionsMenu/OptionsMenuSeparator.tsx	85.71% <ø> (ø)	⬆️
...e/src/components/OptionsMenu/OptionsMenuToggle.tsx	42.85% <ø> (-19.05%)	⬇️
...ore/src/components/OptionsMenu/OptionsMenuItem.tsx	62.5% <ø> (ø)	⬆️
...mponents/OptionsMenu/OptionsMenuToggleWithText.tsx	39.28% <0%> (-3.58%)	⬇️
...ct-core/src/components/OptionsMenu/OptionsMenu.tsx	73.91% <0%> (-3.36%)	⬇️
...re/src/components/Dropdown/DropdownWithContext.tsx	90.47% <100%> (+0.15%)	⬆️
...rc/components/OptionsMenu/OptionsMenuItemGroup.tsx	69.23% <71.42%> (+12.08%)	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12a6eb5...98aed1c. Read the comment docs.

I'm having trouble using keyboard navigation. When I press enter, focus is not placed on the options. Thus, I cannot use the up/down arrows to navigate the grouped options. This keyboard navigation appears to work with the other option examples.

This looks good to me, @nicolethoen . But just one question. Can I apply grouped items with titles to either a single select or multi-select option list? If so then I'm good to approve.

should we change this to label to match the app launcher and dropdown components?

i think that would be a breaking change at this point...

Isn't this an internal Class? Changing the name should be ok if that that is so. We already changed the name of the Class itself correct?

It's not an internal class, so I actually might need to change the name back.

if isGrouped, the .pf-c-options-menu__menu should be a <div> instead of a <ul>. The dropdown and app launcher work that way if it helps to use as a reference.

hm... the menu is built using the dropdown pieces so i'm surprised that didn't behavior come for free. Nice catch. I'll look into it

Isn't this an internal Class? Changing the name should be ok if that that is so. We already changed the name of the Class itself correct?

LGTM

👍 Thanks @nicolethoen!

Just a note, if you pull in core v2.44.3, it will fix some extra space that shows up above group titles.

This is what it looks like now:

This is what it should look like:

keyboard navigation appears to be working now. Thanks!

nicolethoen added the css review label Nov 27, 2019

nicolethoen requested review from kmcfaul, mcarrano and mcoker November 27, 2019 21:29

dlabrecq suggested changes Nov 30, 2019

View reviewed changes

nicolethoen force-pushed the options_menu_groups branch from 1e41dd7 to bd0951d Compare December 2, 2019 13:15

mcarrano reviewed Dec 3, 2019

View reviewed changes

mcoker reviewed Dec 3, 2019

View reviewed changes

nicolethoen force-pushed the options_menu_groups branch from bd0951d to ecad001 Compare December 4, 2019 14:15

tlabaj suggested changes Dec 5, 2019

View reviewed changes

nicolethoen added 3 commits December 9, 2019 09:19

fix(Options menu): added support for groups and titles

68d28ae

fix keyboard interaction in grouped example

fd95973

menu is div not ul if grouped

ec91dcf

nicolethoen force-pushed the options_menu_groups branch from ecad001 to 94a7302 Compare December 9, 2019 14:19

change OptionsMenuGroup back to OptionsMenuItem and fix sentence case

98aed1c

nicolethoen force-pushed the options_menu_groups branch from 94a7302 to 98aed1c Compare December 9, 2019 14:44

tlabaj approved these changes Dec 9, 2019

View reviewed changes

mcoker approved these changes Dec 11, 2019

View reviewed changes

dlabrecq approved these changes Dec 13, 2019

View reviewed changes

dlabrecq merged commit e608e60 into patternfly:master Dec 13, 2019

tlabaj mentioned this pull request Dec 13, 2019

Options Menu - support for groups and titles #3338

Closed

nicolethoen deleted the options_menu_groups branch February 8, 2023 13:54

+                /** Provides an accessible name for the Options menu items group */
+                ariaLabel?: string;
+                /** TODO */
+                groupTitle?: string | React.ReactNode;

+                      menuItems={menuGroups}
+                      toggle={toggle}
+                      isOpen={isOpen}
+                      isGrouped />

+                /** Provides an accessible name for the Options menu items group */
+                ariaLabel?: string;
+                /** TODO */
+                groupTitle?: string | React.ReactNode;

fix(Options menu): added support for groups and titles by nicolethoen · Pull Request #3360 · patternfly/patternfly-react · GitHub

Uh oh!

fix(Options menu): added support for groups and titles - #3360

fix(Options menu): added support for groups and titles#3360dlabrecq merged 4 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom nicolethoen:options_menu_groupsnicolethoen/patternfly-react:options_menu_groupsCopy head branch name to clipboard

Conversation

nicolethoen commented Nov 27, 2019

Uh oh!

patternfly-build commented Nov 27, 2019

Uh oh!

codecov-io commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

nicolethoen Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

tlabaj Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

nicolethoen Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

mcoker Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

nicolethoen Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tlabaj Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fix(Options menu): added support for groups and titles#3360
dlabrecq merged 4 commits into
patternfly:masterfrom
nicolethoen:options_menu_groups

codecov-io commented Nov 27, 2019 •
edited
Loading