feat: Animate side navigation collapse and expandable section reveal - #4867
feat: Animate side navigation collapse and expandable section reveal#4867jkuelz wants to merge 8 commits into
Conversation
efc4551 to
0e9a964
Compare
b7b961d to
b21c945
Compare
0e9a964 to
8bb3e7c
Compare
b21c945 to
68c46e2
Compare
| } | ||
| __internalRootRef={__internalRootRef} | ||
| > | ||
| <CSSTransition in={expanded} timeout={30} classNames={{ enter: styles['content-enter'] }} nodeRef={ref}> |
There was a problem hiding this comment.
Couldn't use CSSTransition for reveal animations because onExit fires one render after expanded becomes false... so when !expanded=true AND !exitInProgress=true, the formula yields true momentarily causing a flash when padding was removed.
There was a problem hiding this comment.
I don't fully understand. Is this an issue with CSSTransition?
There was a problem hiding this comment.
After looking into this again, the issue was not unique to CSSTransition as I originally thought. It was more about how it was configured. The original had a timeout of 30ms which is far shorter than any animation, causing the 'exited' state to render true while the animations were still in progress, resulting in a mixed transient flash state.
I found that we already had an internal wrapper around React Transition Group’s Transition component, so I used that instead. It exposes the transition status, handles reduced motion, and uses the actual transitionend event when no timeout is provided. This lets the "settled" state reset synchronously from expanded on collapse and move to entered only after the root’s grid-template-rows transition finishes, without adding another custom listener here.
| header={header} | ||
| variant={variant === 'stacked' ? 'stacked' : 'default'} | ||
| disableContentPaddings={disableContentPaddings || !expanded} | ||
| disableContentPaddings={true} |
There was a problem hiding this comment.
Moved padding to ExpandableSection’s .content-inner-body-container, which sits inside the clip and collapses naturally with the grid track. Previously, container's content padding lived outside ExpandableSection's grid transition, so toggling it based on expanded state (as before) caused a visible 1-frame padding jump instead of animating smoothly.
| }: InternalExpandableSectionProps) { | ||
| const ref = useRef<HTMLDivElement>(null); | ||
| const contentInnerRef = useRef<HTMLDivElement>(null); | ||
| // Starts settled when initially expanded or under reduced motion, since neither case |
There was a problem hiding this comment.
or under reduced motion
Where is that part handled?
There was a problem hiding this comment.
Good catch. I think I moved it to css at one point but then didn't add back here. Fixed. The collapse-effect now settles immediately when expanding under reduced motion (previously contentSettled would never become true in that case, since handleContentTransitionEnd only fires on a real transitionend, which never happens when grid-template-rows doesn't transition).
| } | ||
| } | ||
|
|
||
| .link-group-children-inner { |
There was a problem hiding this comment.
If the transition is gated, why do we need to add this rule outside of the one theme rule? Will this cause a regression for VR, e.g. focus ring clipping?
There was a problem hiding this comment.
The focus ring clipping was mostly an issue on expandable section because of how it does its padding/margin, but you're right to flag this here too. These can't be gated to one-theme because we no longer rely on conditional rendering that used to fully unmount LinkGroup's children in every theme. So VR now needs this CSS unconditionally to visually hide the always-mounted content when collapsed.
That said, overflow-y: clip was permanent, in every theme and every motion state, with nothing ever switching it back to visible once expanded. That meant any active/hover/focus state on the first/last child that visually extends past its own box was clipped, permanently, any time the group was expanded. Fixed by adding the same settled-state pattern expandable-section uses.
| const tokens: StyleDictionary.MotionDictionary = { | ||
| motionDurationFast: { default: '110ms', disabled: '0ms' }, | ||
| motionDurationModerate: { default: '150ms', disabled: '0ms' }, | ||
| motionDurationSlow: { default: '200ms', disabled: '0ms' }, |
There was a problem hiding this comment.
This values are different from VR. Changing these tokens here will change animations for all components that use these tokens in One Theme. Is this intentional?
There was a problem hiding this comment.
Yeas this was intentional, and seen as a two way door. I needed to increase the durations to make the animations more impactful than what we have defined in VR, which are generally very quick and snappy. I decided to change them here so that all animations still work together as a system/unit (scaled them up equally ~20ms each). I did a brief gut check on a few components, but I'll admit I didn't go and manually test every instance of motion one by one. I figured these will likely be changing again with our animation story anyway, so it felt like a low risk change to make. and changes this small are usually not very noticeable to the eye.
There was a problem hiding this comment.
Can we decouple the navigation motion from these? We'll have more motion in place. Maybe we should think of motion tokens the same way we do for component specific color tokens. Wdyt?
There was a problem hiding this comment.
I think ideally any component specific motion tokens are more of composites vs individual values. However, this requires a change to theme-builder to be able to parse multiple { } values inside a token value. For now, I created more specific tokens that are used in these two components.
Replace the display:none content toggle in ExpandableSection with a grid-template-rows reveal, and animate the SideNavigation collapse/expand: text fades while icons stay centered, spatial and opacity transitions are staggered with tunable local timing vars, groups get collapsed-state spacing, and hidden content is inert for accessibility. WIP checkpoint on dev-v3-jkuelz-side-nav-animations-refactor so the existing icon-layout PR stays clean.
…stacking with nested groups
4538729 to
073e543
Compare
| </div> | ||
| <SideNavigation | ||
| // header={{ | ||
| // href: '#/', |
There was a problem hiding this comment.
No, good catch, this was just used during testing. Removed.
| @@ -1,30 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Why did we move this to src/expandable-section/styles.scss? It would be good to have a separation if possible to make it more maintainable in the future.
There was a problem hiding this comment.
I discovered that the motion.scss file had drifted from the intended implementation: it targeted old selectors, which had I guess at some point been removed from the main file, making them obsolete and regress on the animations that should've been present in VR.
In addition to restoring those animations, I moved them to the main file because keeping those transition declarations beside the selectors which enable them makes their relationship explicit and avoids another selector/transition divergence in the future.
|
|
||
| // One-theme only: animated grid-template-rows reveal instead of the true/false display:none. VR keeps | ||
| // instantaneous display toggling, to prevent breaking reliance on synchronous isDisplayed()/isExisting() checks. | ||
| @include theming.one-theme-only { |
There was a problem hiding this comment.
If our linter rules allow it, can we wrap all one theme specific styles in one big @include theming.one-theme-only? Makes it easier for us to keep track and maintain these.
There was a problem hiding this comment.
Linter doesn't allow it :/
|
|
||
| const handleContentTransitionEnd = useCallback( | ||
| (event: React.TransitionEvent<HTMLDivElement>) => { | ||
| if (event.propertyName === 'grid-template-rows' && expanded) { |
There was a problem hiding this comment.
I think since we can have nested expandable sections (not sure if we should have, that's a different topic), this condition fires for children too.
073e543 to
14ce6b3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4867 +/- ##
==========================================
+ Coverage 97.64% 97.66% +0.02%
==========================================
Files 957 958 +1
Lines 31195 31299 +104
Branches 11500 11557 +57
==========================================
+ Hits 30459 30567 +108
+ Misses 729 686 -43
- Partials 7 46 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Description
Animates
SideNavigation's collapsed icon rail transition andExpandableSection's content reveal, scoped to 1T only as per the animation strategy and to prevent breaking changes.ExpandableSection'sdisplay: none/blockcontent toggle with an animatedgrid-template-rowsreveal. The content stays mounted and inert while collapsed, to allow for smooth transition states that weren't possible with the previousdisplay: none.grid-template-rowsstrategy as expandable sections to enable transitions: text fade, spatial collapse/expand, and staggered enter/exit timing for links, sections, section-groups, link-groups, and expandable-link-groups.Other behavior changes:
inert(not just visual hiding) to remove their headers/parent links from the tab order and assistive technology when they collapse to zero size in the rail.ariaLabelplumbing andaria-hiddenon section/section-group header text for collapsed-rail accessibility.Why animations only in 1T?
The initial version of this animation applied to all themes and broke the pre-existing contract for ExpandableSection that collapsed/hidden content is visually removed instantly (display: none) — which means any downstream tests that asserted isDisplayed()/isExisting() immediately after a collapse toggle with no wait, could fail. Scoping the animation to one-theme (opt-in) avoids that regression for existing VR/classic consumers while still shipping the new motion for one-theme.
Related links, issue #, if available: n/a
How has this been tested?
dev pipeline
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.