8000 feat(nav): style drilldown menu for nav by srambach · Pull Request #6982 · patternfly/patternfly · GitHub
[go: up one dir, main page]

Skip to content

feat(nav): style drilldown menu for nav#6982

Merged
mcoker merged 4 commits intopatternfly:v6from
srambach:6919-nav-drilldown-redesign
Aug 19, 2024
Merged

feat(nav): style drilldown menu for nav#6982
mcoker merged 4 commits intopatternfly:v6from
srambach:6919-nav-drilldown-redesign

Conversation

@srambach
Copy link
Member

Fixes #6919

@patternfly-build
Copy link
Collaborator
patternfly-build commented Aug 15, 2024

@srambach srambach linked an issue Aug 15, 2024 that may be closed by this pull request
Copy link
Collaborator
@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

fire away

Copy link
Contributor
@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

Nice! LGTM, just some nit comments.

Comment on lines +149 to +150
--#{$menu}__list-item--hover--BackgroundColor: var(--#{$nav}__link--hover--BackgroundColor);
--#{$menu}__list-item--hover--BackgroundColor: transparent;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this one is duped

Comment on lines +160 to +163
&:hover,
&:focus {
--#{$menu}__item--BackgroundColor: var(--#{$nav}__link--hover--BackgroundColor);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious if we could remove this and get the same with --pf-v6-c-menu__list-item--hover--BackgroundColor: var(--pf-v6-c-nav__link--hover--BackgroundColor); in the &.pf-m-drilldown above? We would then want to update the border-radius so it goes on .pf-v6-c-menu__list-item and .pf-v6-c-menu__list-item::before (or maybe just .pf-v6-c-menu__list-item::before if that works?) and clean up the existing item/list-item background vars on &.pf-m-drilldown

--#{$menu}__item--BackgroundColor: var(--#{$nav}__link--BackgroundColor);

.#{$menu}__list {
gap: var(--#{$nav}__subnav--RowGap);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so we don't need to change this if we ended up adding a column-gap for some reason

Suggested change
gap: var(--#{$nav}__subnav--RowGap);
row-gap: var(--#{$nav}__subnav--RowGap);

@srambach srambach requested a review from mcoker August 19, 2024 12:33
Copy link
Contributor
@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM! 🥳 Also built locally on react and looks good there, too.

@mcoker mcoker merged commit 876119f into patternfly:v6 Aug 19, 2024
@patternfly-build
Copy link
Collaborator

🎉 This PR is included in version 6.0.0-alpha.216 🎉

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nav - drilldown needs to be updated for v6

4 participants

0