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

feat(nav): added drilldown menu to nav #4458

Merged
merged 4 commits into from Nov 11, 2021

Conversation

mattnolting
Copy link
Contributor

@mattnolting mattnolting commented Oct 20, 2021

closes #4416

For .pf-c-drilldown to function inside of .pf-c-nav, .pf-c-drilldown must replace the internal elements of nav. So .pf-c-nav acts primary as a theming element for .pf-c-drilldown. This means that .pf-c-nav must support pf-c-menu, as a direct child, rather being a child of .pf-c-nav__item.

Nav menu flyouts have different styling than base/standard menu items. This means that a flyout menu must have the ability to be a child of a base/standard menu__item.

@patternfly-build
Copy link

patternfly-build commented Oct 20, 2021

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

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.

Sorry for the delay reviewing! Nice job! 😎 Left some feedback

}
// nested flyout menu
.pf-c-menu.pf-m-flyout,
&.pf-m-flyout .pf-c-menu {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - could this be written as...

  &.pf-m-flyout,
  &.pf-m-flyout & {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't be written as above, the as that would apply to unnested menus. Lmk if this works.

Copy link
Contributor

@mcoker mcoker Nov 10, 2021

Choose a reason for hiding this comment

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

oh I think it would be https://www.sassmeister.com/gist/7b8731fd4020af2215d69b67a0953a35?token=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&scope=gist,read:user

& &.pf-m-flyout,
&.pf-m-flyout & {
}

doesn't matter though, it doesn't buy us anything and is easier to read the way you've written it.

}

position: relative;

.pf-c-menu {
--pf-c-menu--MinWidth: 100%;
--pf-c-menu--BackgroundColor: var(--pf-global--BackgroundColor--dark-300);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--pf-c-menu--BackgroundColor: var(--pf-global--BackgroundColor--dark-300);
--pf-c-menu--BackgroundColor: var(--pf-c-nav--c-menu--BackgroundColor);

--pf-c-nav__item--m-drilldown--m-expanded__toggle-icon--Rotate: 0;
--pf-c-nav__item--m-drilldown--m-expanded__subnav--ZIndex: var(--pf-global--ZIndex--xs);
// Menu flyout
--pf-c-nav--c-menu--m-flyout--Top: calc(0px + var(--pf-c-nav--c-menu--m-flyout--top-offset));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this not be written without the calc (since x + 0 will always be x), and also with --pf-c-nav--c-menu--m-flyout--top-offset: 0px; set to 0 (remove the stylelint-disable comment) since there isn't a calc?

Suggested change
--pf-c-nav--c-menu--m-flyout--Top: calc(0px + var(--pf-c-nav--c-menu--m-flyout--top-offset));
--pf-c-nav--c-menu--m-flyout--Top: var(--pf-c-nav--c-menu--m-flyout--top-offset);

Copy link
Contributor

Choose a reason for hiding this comment

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

Also not sure if --pf-c-nav--c-menu--m-flyout--top-offset is ever used again, but if not, could this just be set to --pf-c-nav--c-menu--m-flyout--Top: 0?

--pf-c-nav--c-menu--m-flyout--c-menu--Bottom: auto;
--pf-c-nav--c-menu--m-flyout--c-menu--Left: calc(100% - var(--pf-c-nav--c-menu--m-flyout--left-offset));
--pf-c-nav--c-menu--m-flyout--m-left--Right: calc(100% - var(--pf-c-nav--c-menu--m-flyout--m-left--right-offset));
--pf-c-nav--c-menu--m-flyout--m-top--Bottom: calc(0px + var(--pf-c-nav--c-menu--m-flyout--m-top--bottom-offset));
Copy link
Contributor

Choose a reason for hiding this comment

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

same question as the --top-offset declaration

--pf-c-nav--c-menu__list-item--focus-within--Color: var(--pf-global--Color--light-100);
--pf-c-nav--c-menu__list-item--hover--BackgroundColor: var(--pf-global--BackgroundColor--dark-200);
--pf-c-nav--c-menu__list-item--focus-within--BackgroundColor: var(--pf-global--BackgroundColor--dark-200);
--pf-c-nav--c-menu__list-item--active--BackgroundColor: var(--pf-global--BackgroundColor--dark-200);
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't look like this is used


&.pf-m-left {
--pf-c-menu--m-flyout__menu--Right: var(--pf-c-menu--m-flyout__menu--m-left--Right);
--pf-c-menu--m-flyout__menu--Left: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have .pf-m-top and .pf-m-left defined twice in the nav css. Are we able to just define the modifier classes here and use vars in the nav css if we need to change something?

--pf-c-menu__list--PaddingBottom: 0;
--pf-c-menu__item--FontSize: var(--pf-c-nav--c-menu__item--FontSize);
--pf-c-menu__item--Color: var(--pf-c-nav--c-menu__item--Color);
--pf-c-menu__item--PaddingTop: var(--pf-global--spacer--sm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--pf-c-menu__item--PaddingTop: var(--pf-global--spacer--sm);
--pf-c-menu__item--PaddingTop: var(--pf-c-nav--c-menu--m-flyout__item--PaddingTop);

--pf-c-menu__item--Color: var(--pf-c-nav--c-menu__item--Color);
--pf-c-menu__item--PaddingTop: var(--pf-global--spacer--sm);
--pf-c-menu__item--PaddingRight: var(--pf-c-nav--c-menu--m-flyout__item--PaddingRight);
--pf-c-menu__item--PaddingBottom: var(--pf-global--spacer--sm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--pf-c-menu__item--PaddingBottom: var(--pf-global--spacer--sm);
--pf-c-menu__item--PaddingBottom: var(--pf-c-nav--c-menu--m-flyout__item--PaddingBottom);

@mcoker
Copy link
Contributor

mcoker commented Nov 4, 2021

Also looks like you have some scrollbar action going on in the drilldown variant Screen Shot 2021-11-04 at 3 08 32 PM

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.

Looks great! Just a couple of nits and looks like there is a positioning issue with the examples
Screen Shot 2021-11-10 at 1 05 54 PM

src/patternfly/components/Menu/menu.scss Outdated Show resolved Hide resolved
src/patternfly/components/Nav/nav.scss Outdated Show resolved Hide resolved
@mcoker
Copy link
Contributor

mcoker commented Nov 10, 2021

Looks like there is some extra space and scrollbar in the menu drilldown examples now:

Screen Shot 2021-11-10 at 4 35 20 PM

@mcoker mcoker merged commit 2e3ac12 into patternfly:main Nov 11, 2021
@patternfly-build
Copy link

🎉 This PR is included in version 4.157.0 🎉

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.

Update nav drilldown to use menu
3 participants