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

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

Merged
merged 4 commits into from Dec 13, 2019

Conversation

@nicolethoen
Copy link
Contributor

nicolethoen commented Nov 27, 2019

Addresses #3338

Follow up to core PR #2403

@nicolethoen nicolethoen requested review from mcarrano, mcoker and kmcfaul Nov 27, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 27, 2019

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 27, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Member

dlabrecq left a comment

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.

@nicolethoen nicolethoen force-pushed the nicolethoen:options_menu_groups branch from 1e41dd7 to bd0951d Dec 2, 2019
Copy link
Member

mcarrano left a comment

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.

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

This comment has been minimized.

Copy link
@mcoker

mcoker Dec 3, 2019

Contributor

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

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Dec 3, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@tlabaj

tlabaj Dec 5, 2019

Contributor

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?

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Dec 6, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@mcoker

mcoker Dec 3, 2019

Contributor

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.

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Dec 3, 2019

Author Contributor

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

@nicolethoen nicolethoen force-pushed the nicolethoen:options_menu_groups branch from bd0951d to ecad001 Dec 4, 2019
/** Provides an accessible name for the Options menu items group */
ariaLabel?: string;
/** TODO */
groupTitle?: string | React.ReactNode;

This comment has been minimized.

Copy link
@tlabaj

tlabaj Dec 5, 2019

Contributor

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?

@nicolethoen nicolethoen force-pushed the nicolethoen:options_menu_groups branch from ecad001 to 94a7302 Dec 9, 2019
@nicolethoen nicolethoen force-pushed the nicolethoen:options_menu_groups branch from 94a7302 to 98aed1c Dec 9, 2019
@tlabaj
tlabaj approved these changes Dec 9, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@mcoker
mcoker approved these changes Dec 11, 2019
Copy link
Contributor

mcoker left a comment

👍 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:
Screen Shot 2019-12-11 at 1 49 04 PM

This is what it should look like:
Screen Shot 2019-12-11 at 1 48 53 PM

Copy link
Member

dlabrecq left a comment

keyboard navigation appears to be working now. Thanks!

@dlabrecq dlabrecq merged commit e608e60 into patternfly:master Dec 13, 2019
9 checks passed
9 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_a11y_pf4 Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.