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(menu): removed pf-m-drilldown transition, fixed content height transition #4090

Merged
merged 3 commits into from May 25, 2021

Conversation

mattnolting
Copy link
Contributor

closes #4062

@patternfly-build
Copy link

patternfly-build commented May 21, 2021

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

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

CSS Size Report
NameCurrentPreviousDiff %
components/Menu/menu.css16.8 kB16.9 kB-0.79
patternfly-no-reset.css1.2 MB1.2 MB-0.01
patternfly.css1.2 MB1.2 MB-0.01
patternfly.min.css1.0 MB1.0 MB-0.01

@@ -173,23 +169,37 @@
&.pf-m-drilldown {
display: flex;
flex-direction: column;
transition: var(--pf-c-menu--m-drilldown--Transition);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this change will fix the original issue. PopperJS is applying transform: translate3d(...) to the outer .pf-c-menu to position the menu relative to the toggle, and this transition on .pf-c-menu.pf-m-drilldown was causing that position to transition in. You can see it if you add className="pf-m-drilldown" to one of the <Menu> components in the composable menu demos.

Unless there's another reason to transition left on the inner menus, I think transitioning translate() is better because it's more performant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also an aside, I found this in the popperJS docs - looks like you may be able to change the way popper positions the menu to stick with top and left positioning if we still have conflicting styles - https://popper.js.org/docs/v2/modifiers/compute-styles/#gpuacceleration.


.pf-c-menu,
.pf-c-menu__list {
transition: var(--pf-c-menu--m-drilldown--Transition);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this var be renamed?

@mattnolting mattnolting force-pushed the refactor-menu-issue-4062 branch 3 times, most recently from 9adbda8 to 3222b8b Compare May 25, 2021 02:38
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.

L🔥TM!

@mcoker mcoker merged commit 182cd50 into patternfly:master May 25, 2021
@mcoker mcoker changed the title refactor(menu): changed translate to left fix(menu): removed pf-m-drilldown transition, fixed content height transition May 25, 2021
@patternfly-build
Copy link

🎉 This PR is included in version 4.104.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.

Menu component positioned with popper transitions unexpectedly
3 participants