feat(nav): animate expandable nav items#7321
Conversation
|
Preview: https://patternfly-pr-7321.surge.sh A11y report: https://patternfly-pr-7321-a11y.surge.sh |
|
The opening animation looks great but closing feels a bit clunky. It feels like there is a pause between the sub items collapsing and the nav repositioning. |
There was a problem hiding this comment.
As discussed in standup, this is just a general review of the code, not yet a review of the animation work specifically 👍
src/patternfly/base/normalize.scss
Outdated
| line-height: var(--pf-t--global--font--line-height--body); | ||
| color: var(--pf-t--global--text--color--regular); | ||
| // stylelint-disable property-no-unknown | ||
| interpolate-size: allow-keywords; |
There was a problem hiding this comment.
This is something we'll need to keep a close eye on as it's only supported in chromium browsers at the moment - https://caniuse.com/mdn-css_properties_interpolate-size_allow-keywords
It has the potential to make us and users believe keyword animations are working fine everywhere if the animation is not tested in FF/Safari.
There was a problem hiding this comment.
I'll remove this as I don't think it's necessary now.
| // * Nav toggle icon | ||
| // * Nav item | ||
| --#{$nav}__item--RowGap: var(--#{$nav}__list--RowGap); | ||
| --#{$nav}__item--TransitionDuration: 0; // var(--pf-t--global--motion--duration--slide-in--default); |
There was a problem hiding this comment.
Just a nit, but did you mean to leave the comment? I'd probably take it out - one less thing to track/update if we changed the token used.
| --#{$nav}__link--background-color--TransitionDuration: var(--pf-t--global--motion--duration--fade--default); | ||
| --#{$nav}__link--background-color--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); | ||
|
|
||
| // text color transition for current | ||
| --#{$nav}__link--m-current--color--TransitionDuration: var(--pf-t--global--motion--duration--fade--short); | ||
| --#{$nav}__link--m-current--color--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); |
There was a problem hiding this comment.
Nit, looking at a couple of other components, we're adding the property for the transition in the var name after Transition[Duration/Timing/etc]
patternfly/src/patternfly/components/Menu/menu.scss
Lines 130 to 131 in 917bda0
patternfly/src/patternfly/components/Tabs/tabs.scss
Lines 112 to 114 in 917bda0
| --#{$nav}__link--background-color--TransitionDuration: var(--pf-t--global--motion--duration--fade--default); | |
| --#{$nav}__link--background-color--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); | |
| // text color transition for current | |
| --#{$nav}__link--m-current--color--TransitionDuration: var(--pf-t--global--motion--duration--fade--short); | |
| --#{$nav}__link--m-current--color--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); | |
| --#{$nav}__link--TransitionDuration--background-color: var(--pf-t--global--motion--duration--fade--default); | |
| --#{$nav}__link--TransitionTimingFunction--background-color: var(--pf-t--global--motion--timing-function--default); | |
| // text color transition for current | |
| --#{$nav}__link--m-current--TransitionDuration--color: var(--pf-t--global--motion--duration--fade--short); | |
| --#{$nav}__link--m-current--TransitionTimingFunction--color: var(--pf-t--global--motion--timing-function--default); |
| --#{$nav}__subnav--max-height--TransitionDuration: 0s; // no slide by default | ||
| --#{$nav}__subnav--max-height--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); |
There was a problem hiding this comment.
| --#{$nav}__subnav--max-height--TransitionDuration: 0s; // no slide by default | |
| --#{$nav}__subnav--max-height--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); | |
| --#{$nav}__subnav--TransitionDuration--max-height: 0s; // no slide by default | |
| --#{$nav}__subnav--TransitionTimingFunction--max-height: var(--pf-t--global--motion--timing-function--default); |
| @media ( prefers-reduced-motion: no-preference ) { | ||
| --#{$nav}__subnav--max-height--TransitionDuration: var(--pf-t--global--motion--duration--slide-in--default); | ||
| } |
There was a problem hiding this comment.
| @media ( prefers-reduced-motion: no-preference ) { | |
| --#{$nav}__subnav--max-height--TransitionDuration: var(--pf-t--global--motion--duration--slide-in--default); | |
| } | |
| @media (prefers-reduced-motion: no-preference) { | |
| --#{$nav}__subnav--TransitionDuration--max-height: var(--pf-t--global--motion--duration--slide-in--default); | |
| } |
| } | ||
|
|
||
| &.pf-m-expanded { | ||
| row-gap: var(--#{$nav}__item--RowGap); // no fallback here as this value is used to calc clickable offsets |
There was a problem hiding this comment.
nit but you can remove that comment. It's leftover from the PR that made nav items clickable outside of the visible area
| row-gap: var(--#{$nav}__item--RowGap); // no fallback here as this value is used to calc clickable offsets | |
| row-gap: var(--#{$nav}__item--RowGap); |
| grid-template-rows: 0fr; | ||
| } | ||
|
|
||
| [hidden] { |
There was a problem hiding this comment.
We'll ideally scope this to an element, otherwise it could target something we don't intend. Also just to confirm, the element it's supposed to target gets hidden in react, versus being removed or something else? A lot of things in core are hidden when they're just removed or something else in react.
There was a problem hiding this comment.
This compiles out to this snippet - did you mean scoped further than a subnav? And yes, it seems to just be [hidden] - but maybe I'm missing something?
.pf-v6-c-nav__subnav [hidden] { --pf-v6-c-nav__list--RowGap: 0; grid-template-rows: 0fr; }
There was a problem hiding this comment.
I believe it should be as such so that it's associated with .pf-v6-c-nav__subnav
| [hidden] { | |
| &[hidden] { |
I tried it locally and the subnav collapse is much smoother.
| border-radius: var(--#{$nav}__link--BorderRadius); | ||
| transition-timing-function: var(--#{$nav}__link--background-color--TransitionTimingFunction), var(--#{$nav}__link--m-current--color--TransitionTimingFunction); | ||
| transition-duration: var(--#{$nav}__link--background-color--TransitionDuration), var(--#{$nav}__link--m-current--color--TransitionDuration); | ||
| transition-property: background-color color; |
There was a problem hiding this comment.
| transition-property: background-color color; | |
| transition-property: background-color, color; |
| transition-timing-function: var(--#{$nav}__link--background-color--TransitionTimingFunction), var(--#{$nav}__link--m-current--color--TransitionTimingFunction); | ||
| transition-duration: var(--#{$nav}__link--background-color--TransitionDuration), var(--#{$nav}__link--m-current--color--TransitionDuration); |
There was a problem hiding this comment.
| transition-timing-function: var(--#{$nav}__link--background-color--TransitionTimingFunction), var(--#{$nav}__link--m-current--color--TransitionTimingFunction); | |
| transition-duration: var(--#{$nav}__link--background-color--TransitionDuration), var(--#{$nav}__link--m-current--color--TransitionDuration); | |
| transition-timing-function: var(--#{$nav}__link--TransitionTimingFunction--background-color), var(--#{$nav}__link--m-current--TransitionTimingFunction--color); | |
| transition-duration: var(--#{$nav}__link--TransitionDuration--background-color), var(--#{$nav}__link--m-current--TransitionDuration--color); |
| --#{$nav}__item--TransitionDuration: 0; // var(--pf-t--global--motion--duration--slide-in--default); | ||
| --#{$nav}__item--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); | ||
|
|
||
| @media ( prefers-reduced-motion: no-preference ) { |
There was a problem hiding this comment.
🥸
| @media ( prefers-reduced-motion: no-preference ) { | |
| @media (prefers-reduced-motion: no-preference) { |
@andrew-ronaldson That's actually a question I had. If you look at the timeline, opacity happens in about half the time of the slide. WDYT about making them the same? Here's what was spec'd, and I realized I also need to add in close timing function (accelerate - but the difference in duration remains) |
|
@andrew-ronaldson Also, here's a video with the opacity duration of the list set to the same value as the slide duration 2025-02-04_14-16-20.mp4 |
|
referencing the video above: I like the the opacity duration of the list set to the same value as the slide duration |
|
Fixed the incorrect selector for a hidden subnav. Based on discussion at 2/6/25 animation meeting, we'll leave the duration of the opacity transition the same as the others |
There was a problem hiding this comment.
Beauty in motion. 📽️
|
@andrew-ronaldson @kaylachumley - @srambach changed the item spacing a bit to accommodate the change to the focus ring. Would you mind reviewing again? |
There was a problem hiding this comment.
Let's move and groove!
There was a problem hiding this comment.
NICE! Left some comments, some we talked about. Looks like you don't need to write animations to remove the @starting-style blocks? Left some comments 👍
| display: none; | ||
| visibility: hidden; | ||
|
|
||
| & .#{$nav}__list { |
There was a problem hiding this comment.
nit - the & is redundant here
| & .#{$nav}__list { | |
| .#{$nav}__list { |
| padding-block-start: 0; | ||
| padding-block-end: 0; |
There was a problem hiding this comment.
If you add --#{$nav}__subnav--PaddingBlock[Start/End]: --#{$nav}__item--RowGap as base vars and use them here, you can modify the vars under &.pf-m-expanded instead of having to add a block for & > .#{$nav}__subnav and declare the padding properties here
patternfly/src/patternfly/components/Nav/nav.scss
Lines 267 to 274 in b635f76
| transition-timing-function: var(--#{$nav}__list--TransitionTimingFunction--opacity); | ||
| transition-duration: var(--#{$nav}__list--TransitionDuration--opacity); | ||
| transition-property: opacity; | ||
| transition-behavior: allow-discrete; |
There was a problem hiding this comment.
Doesn't look like you need this now
| transition-timing-function: var(--#{$nav}__subnav--TransitionTimingFunction--grid-template-rows); | ||
| transition-duration: var(--#{$nav}__subnav--TransitionDuration--grid-template-rows); | ||
| transition-property: grid-template-rows, padding-block-start; | ||
| transition-behavior: allow-discrete; |
There was a problem hiding this comment.
Looks like you can remove this now
| .#{$nav}__toggle-icon { | ||
| display: inline-block; | ||
| transition-duration: var(--#{$nav}__item__toggle-icon--TransitionDuration--transform); | ||
| transition-property: transform; |
There was a problem hiding this comment.
Does this need a timing function?
| @starting-style { | ||
| grid-template-rows: 0fr; | ||
| } |
There was a problem hiding this comment.
I don't think you need this actually? It starts out as 0fr already
patternfly/src/patternfly/components/Nav/nav.scss
Lines 246 to 250 in b635f76
There was a problem hiding this comment.
Are you saying we don't need the @starting-style block? That allows the animation to run on page load (same with the next comment, if that's what you meant.)
There was a problem hiding this comment.
Ah! I didn't know those were for firing on page load. I'll re-review 👍
| @starting-style { | ||
| opacity: 0; | ||
| } |
There was a problem hiding this comment.
You shouldn't need this, either. It's already 0 when the subnav is hidden
patternfly/src/patternfly/components/Nav/nav.scss
Lines 189 to 197 in b635f76
There was a problem hiding this comment.
Discussed offline but the subnav base vars seemed like they might be able to be a little simpler? I proposed a couple of changes in a PR to this PR - srambach#4. It also moves the opacity animation to the subnav instead of list, so the expand/fade is all on the one containing element now, which will work better if we add other stuff to subnav (groups with titles, for example)
Combine transition vars and fix spacing, move opacity to subnav
There was a problem hiding this comment.
Let's move this into the Done column!
|
🎉 This PR is included in version 6.2.0-prerelease.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #6597
Implements microanimations on the nav component:
Figma reference: https://www.figma.com/design/VMEX8Xg2nzhBX8rfBx53jp/branch/UR1JfYwpZI6MP3rLj18iJ3/PatternFly-6%3A-Components?m=auto&node-id=3-35&t=PQCNAH226TGUdUfd-1
TIP - the site navigation will reflect the changes, rather than having to go to the examples and manually manipulate them.