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(drilldown): added drilldown menu #3438

Merged
merged 11 commits into from
Oct 21, 2020

Conversation

mattnolting
Copy link
Contributor

@mattnolting mattnolting commented Aug 27, 2020

fixes #2635

Parent menus control placement of children. If pf-m-drilled-in is applied to pf-c-menu, the menu shifts to the left transform: translateX(-100%), out of view. The nested, children menus are positioned left: 100%;, until .pf-m-drilled-in is applied, where then transform: translateX(-100%) is applied.

Immediate parent li of the current menu has .pf-m-current-path applied. pf-m-current-path hides sibling, non-current li s and sets their visibility to hidden. We need to use visibility here because JS will need to reference height of lists when showing/hiding them.

@patternfly-build
Copy link

patternfly-build commented Aug 27, 2020

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

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

CSS Size Report
NameCurrentPreviousDiff %
components/Menu/menu.css12.6 kB9.4 kB25.17
patternfly-no-reset.css888.9 kB885.7 kB0.36
patternfly.css890.8 kB887.6 kB0.36
patternfly.min.css788.3 kB785.5 kB0.36

@mattnolting
Copy link
Contributor Author

@mcoker @christiemolloy examples have included JS to replicate React state mgmt. I'll remove that before final review.

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 is looking good @mattnolting . My only critique is that when you drill into a new level, I believe that the header should show the name of the parent node rather than just the text "Back". To return up a level, the user would click the "<" icon. Does that make sense to others?

@doruskova
Copy link

@mattnolting @mcarrano Agree with showing the current level and use the back arrow for going back. However, it looks great to me. :)

@mcoker
Copy link
Contributor

mcoker commented Sep 15, 2020

To return up a level, the user would click the "<" icon.

@mcarrano do you mean only the arrow would be clickable, and not the entire item? (arrow + parent node name)

@mcoker
Copy link
Contributor

mcoker commented Sep 15, 2020

This looks great! I'll dig in some more, but some initial questions:

  1. Do we need to animate the height? or could we just animate translate/opacity of menus coming in/out from the left/right? That would simplify the logic and increase the performance of the animation.
  2. Not sure about the width… if we keep it, need to use pf-size-prem(). Did you try letting the width be determined by the contents of the menu?
  3. Mentioned this outside of the issue, but I think we should be specific about the properties we animate on which items. Currently we're animating "all" on everything and on items that I wonder if need animation, like __menu-item.

@mattnolting
Copy link
Contributor Author

mattnolting commented Sep 15, 2020

  1. Do we need to animate the height? or could we just animate translate/opacity of menus coming in/out from the left/right? That would simplify the logic and increase the performance of the animation.

To accomplish the left/right transition, I had to hide overflow, limiting the viewable children to the root dropdown menu width. Which means using the parent menu as the background for all child menus. Otherwise you would see child menus fading into view to the left/right of the initial dropdown. So it would be more of a fade in/fade out transition than a slide in/slide out transition. I couldn't find another way do that, also the dropshadow interferes with transitions when using the child menu's background.

  1. Not sure about the width… if we keep it, need to use pf-size-prem(). Did you try letting the width be determined by the contents of the menu?

Since I used the width of the parent menu for all children, I had to set a base width. We could apply that css variable to the menu component making it easily changeable and set the base size w/pf-size-prem().

  1. Mentioned this outside of the issue, but I think we should be specific about the properties we animate on which items. Currently we're animating "all" on everything and on items that I wonder if need animation, like __menu-item.

Yep, agree 👍

Here's the prototype I created if you want to experiment with it : https://codepen.io/bobcatTaco/pen/pogQbmY

@mcarrano
Copy link
Member

This is looking great. I love the animation @mattnolting ! The only thing I would change is what shows up in the breadcrumb header when you drill down. Rather than being just a back link, it would contain the title of the parent node that you are viewing the contents of. The user would need to click the "<" to go back up a level. See how the Move To feature works in Google Drive, for example. Here's what that looks like:

Screen Shot 2020-09-16 at 11 27 17 AM

Only the back-arrow is interactive.

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.

This looks good to me! Can you update the transitions to be more specific?

@mattnolting mattnolting force-pushed the feat-drilldown branch 4 times, most recently from e716188 to d33c324 Compare September 30, 2020 18:21
@mattnolting mattnolting force-pushed the feat-drilldown branch 3 times, most recently from ebdfd30 to addefef Compare October 15, 2020 18:33
@mattnolting
Copy link
Contributor Author

mattnolting commented Oct 15, 2020

Looking at the JS interactivity, I noticed when I click on a menu item, it opens the next menu very quickly and I feel like I lose my position, could the keyboard focus / tabindex go straight to the top of the menu?

@christiemolloy Great point, i'm not sure how to do this statically. I'd like to shift this to React.

@mattnolting
Copy link
Contributor Author

@christiemolloy @mcoker updated

@@ -2,6 +2,7 @@
{{#if divider--attribute}}
{{divider--attribute}}
{{/if}}
aria-hidden="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to add this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, but maybe better addressed in another issue :)

--pf-c-menu--m-drilldown--c-menu--Transition: transform var(--pf-c-menu--m-drilldown--c-menu--TransitionDuration--transform), visibility var(--pf-c-menu--m-drilldown--c-menu--TransitionDuration--visibility);

// Drilldown list
--pf-c-menu--m-drilldown__list--TransitionDuration--transform: transform var(--pf-global--TransitionDuration);
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--m-drilldown__list--TransitionDuration--transform: transform var(--pf-global--TransitionDuration);
--pf-c-menu--m-drilldown__list--TransitionDuration--transform: var(--pf-global--TransitionDuration);

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.

Just one thing to fix a broken transition, and a question about the divider.

@mattnolting mattnolting force-pushed the feat-drilldown branch 2 times, most recently from e2b82bc to e031697 Compare October 21, 2020 15:08
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!!!

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!!!

@mcoker mcoker merged commit bdb0d66 into patternfly:master Oct 21, 2020
@redallen
Copy link
Contributor

🎉 This PR is included in version 4.55.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mattnolting mattnolting deleted the feat-drilldown branch January 6, 2022 21:37
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.

Need support for multi level drilldown menu
7 participants