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(select): added pf-m-focus support for favorite __menu-items #3431

Merged
merged 2 commits into from Aug 27, 2020

Conversation

mcoker
Copy link
Contributor

@mcoker mcoker commented Aug 25, 2020

fixes #3429

This also adds :focus-within support in the app launcher to match the select, so that when you focus on an element in a favorite menu item, the whole item's background changes. This will change the current keyboard navigation style in the app launcher from:
before

to:
after

This makes the app launcher match the select, and the general style of navigating any of these types of menus by keyboard.

@patternfly-build
Copy link

patternfly-build commented Aug 25, 2020

Preview: https://patternfly-pr-3431.surge.sh

A11y report: https://patternfly-pr-3431-coverage.surge.sh

CSS Size Report
NameCurrentPreviousDiff %
components/Select/select.css21.1 kB20.9 kB1.11
components/AppLauncher/app-launcher.css11.9 kB11.8 kB0.47
patternfly.min.css717.3 kB717.1 kB0.04
patternfly-no-reset.css811.3 kB811.0 kB0.04
patternfly.css813.2 kB812.9 kB0.04

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to make sense. @gdoyle1 can you also take a look to confirm that this works? Thanks.

@mcoker
Copy link
Contributor Author

mcoker commented Aug 26, 2020

@nicolethoen @tlabaj this PR updates the CSS to work with the way the react component adds pf-m-focus to pf-c-select__menu-item. However, in the favorites select menu, the parent li.pf-c-select__menu-wrapper is what we style on :hover and :focus-within to create the grey background behind all of the actions in that item row. We don't apply that styling to the pf-c-select__menu-item element like other menus with a single action per item (when the item is the action).

If it's straight forward to do so, could we move the pf-m-focus class from pf-c-select__menu-items to the parent li.pf-c-select__menu-wrapper element in the react component when navigating by keyboard? If so, I'll need to update this PR. If it's not ideal to do that we can keep it the way it is now in this PR.

@nicolethoen
Copy link
Contributor

nicolethoen commented Aug 27, 2020

@nicolethoen @tlabaj this PR updates the CSS to work with the way the react component adds pf-m-focus to pf-c-select__menu-item. However, in the favorites select menu, the parent li.pf-c-select__menu-wrapper is what we style on :hover and :focus-within to create the grey background behind all of the actions in that item row. We don't apply that styling to the pf-c-select__menu-item element like other menus with a single action per item (when the item is the action).

If it's straight forward to do so, could we move the pf-m-focus class from pf-c-select__menu-items to the parent li.pf-c-select__menu-wrapper element in the react component when navigating by keyboard? If so, I'll need to update this PR. If it's not ideal to do that we can keep it the way it is now in this PR.

This would take a little change on the react side I think, but it shouldn't be problematic.
We have so many pending react changes anyway... lol

Copy link
Contributor

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LPTM 👍

Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcoker mcoker merged commit c4ab7f9 into patternfly:master Aug 27, 2020
@redallen
Copy link
Contributor

🎉 This PR is included in version 4.35.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Select - focused menu items not styled correctly
7 participants