ToggleGroupControl: rewrite backdrop animation with framer-motion #40107
Conversation
| className={ labelViewClasses } | ||
| data-active={ isActive } | ||
| // Necessary for the ToggleGroupControlBackdrop component to render properly | ||
| data-toggle-group-control-option-wrapper="true" |
There was a problem hiding this comment.
The data-toggle-group-control-option-wrapper is used as a way for the backdrop component to always find the correct ToggleGroupControl's "option wrapper", which is used to compute the correct position and size to apply to the backdrop.
| @@ -98,7 +104,9 @@ export type ToggleGroupControlProps = Omit< | |||
| /** | |||
| * React children | |||
| */ | |||
| children: ReactNode; | |||
| children: | |||
There was a problem hiding this comment.
This change is not necessary, but I thought it could be interesting to enforce only the expected children type via TypeScript
| window.cancelAnimationFrame( animationRequestId ); | ||
| }; | ||
| }, [ canAnimate, containerRef, containerWidth, state, isAdaptiveWidth ] ); | ||
| const toggleGroupOptionWrapper = item.ref.current?.closest( |
There was a problem hiding this comment.
Since the markup of ToggleGroupControlOptionBase can change if the tooltip is enabled or not, I decided to add a data attribute to the option's wrapper and use Element.closest() to find it each item's wrapper.
| bounce: 0.2, | ||
| // Transition durations in the config are expressed as a string in milliseconds, | ||
| // while `framer-motion` needs them as integers in seconds. | ||
| duration: parseInt( CONFIG.transitionDurationFast, 10 ) / 1000, |
There was a problem hiding this comment.
This felt a bit hacky but I couldn't come up with another way of reusing our config as it's written today
|
Size Change: -24 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
| @@ -33,7 +33,6 @@ const TOGGLE_GROUP_CONTROL_PROPS = { | |||
| toggleGroupControlBackdropBackgroundColor: | |||
| CONTROL_PROPS.controlSurfaceColor, | |||
| toggleGroupControlBackdropBorderColor: COLORS.ui.border, | |||
| toggleGroupControlBackdropBoxShadow: 'transparent', | |||
There was a problem hiding this comment.
Deleted as it didn't seem like it made any difference
ciampo
added
[Type] Bug
ciampo
force-pushed
the
feat/toggle-group-backdrop-animation-rewrite
branch
from
0ab7c9d
to
931d63e
Compare
|
Very nice! The animation for the backdrop is present in the segmented control in this branch, but presumably without the technical challenges that are present in trunk:
For comparison, here's the trunk version: The trunk version feels a bit slower, just a millisecond or so. That makes it feel a bit more alive — still fast enough that if you use the arrowkeys to set the specific size, it feels instant enough, but still slow enough that you the animation is perceptible. Can we slow this framer version down just a teensy bit to better match what's in trunk? I realize it's probably hard to discern the speed differential with the reduced framerate of the GIFs above, but nevertheless. |
I changed it (ffde8ad) from If that's still too fast, I'm happy to introduce one more variable — maybe |
|
I'd love another inspection on this, because it still feels a bit too fast for me. Compare this branch: with trunk:
Trunk still feels a bit more harmonious. The curious thing is, looking at the code, it appears the trunk version was set to 100ms 🤔 Is there any elastic bounciness going on? It's a bit hard to tell. If yes, I don't think we should have that — ease in/out should be sufficient. Thanks for keeping at this! |
|
I agree that it feels different, it must be a difference between how the animation was implemented previously (raw CSS values) and how
There was indeed some bounciness going on — I went ahead and removed it, while increasing the animation duration to Another thing I noticed is that the previous implementation had a Given that the animation is now slower, would you still like me to re-introduce that delay? I'd personally be against adding a delay here, because it would make the overall interaction feel less responsive (e.g. RAIL model). |
|
The animation actually feels pretty decent now: I noticed another thing. The left side looks good: 1px border, 2px padding inside. But the right side looks off:
|
|
Mhh, I think that could be caused by subpixel rouding/rendering, and can probably vary depending on the device / screen resolution / browser. For example, on my external screen, the font size control component in the editor looks fine in Chrome but similar to your screenshot Firefox: Chrome:
Firefox:
At the same time, the Storybook example seems fine on both Chrome and Firefox: Chrome:
Firefox:
Maybe we can look into it in a separate follow-up, if the rest of this PR looks good? |
|
It doesn't appear to be subpixel math on the backdrop itself, I see all whole pixels:
I know it seems like a small detail, but it bugs me 😅 — if we are to address it in a followup as a bugfix, we need to have high confidence that we can fix it. I.e. if we can figure out a solution, it's fine to do separately. |
|
Alright, I'll take a look at it next week and see what I can do. |
| bounce: 0, | ||
| // Transition durations in the config are expressed as a string in milliseconds, | ||
| // while `framer-motion` needs them as integers in seconds. | ||
| duration: parseInt( CONFIG.transitionDurationSlow, 10 ) / 1000, |
There was a problem hiding this comment.
What do you think about just hardcoding the duration value here? I guess because:
- It's a pretty specialized animation anyway
- What if the
CONFIGvalues are ever changed frommstos - It seems like overkill to extract a separate
TRANSITION_DURATION_MSobject (Or maybe not? Will it come up a lot in these JS animations?)
No strong opinion though, just throwing it out there.
There was a problem hiding this comment.
I'm happy to hardcode it, although we may want to reconsider having a more clear, better organised shared "transition duration" settings when we start refactoring our shared configs
| background: ${ COLORS.gray[ 900 ] }; | ||
| border-radius: ${ CONFIG.controlBorderRadius }; | ||
| box-shadow: ${ CONFIG.toggleGroupControlBackdropBoxShadow }; | ||
| box-shadow: none; |
|
Doh! 🤦 I had meant to make a separate branch but missed it and pushed directly to this one. I hope it's no trouble @ciampo and I'm not trying to make a habit of this! I'm going to leave it for now but if you'd like I can drop the commits and force push this back to how you had it. Before explaining what my changes are about, I want to point out how terribly tricky this seemingly simple animation can be. Marco has done a great job on simplifying it 👏. So the final issue here was the backdrop (the dark box that slides around to indicate the active item) could end up misaligned from the right side of the toggle control’s border #40107 (comment). To fix that we need to have the backdrop use sub-pixel size and positions as previously provided by So what I came up with was to avoid measuring and instead calculate the size and position that the backdrop needs to be. It actually ends up allowing for more simplification but I've just realized the biggest downfall— To support varying sized options, I think measuring will have to be done. Maybe there's a way to apply that strategy only when |
|
I've found a way to support |
|
Thank you for the explanation, @stokesman ! I was also working on a new approach that ideally ditches all calculations altogether by using |
…izes for backdrop
…s only allowed components of ToggleGroupControl
This makes is easy to calculate a width that will be the same as that of the option elements and avoid having to measure them. A few minor style adjustments are included.
And update typings as appropriate.
This reverts commit ed2cd80.
This reverts commit 2ce5baa.
stokesman
force-pushed
the
feat/toggle-group-backdrop-animation-rewrite
branch
from
2ce5baa
to
631d5f9
Compare
|
I've rebased this and updated it so it works with |
|
Thank you @stokesman for the work here! I'd say that the preferred approach for the backdrop animation with With that in mind, I'm going to pause working on/reviewing this PR for the time being, and see if we land #40276 in the next days — in case we can't, we can always get back to this PR. |
|
Closing in favour of #40276 (which is still currently blocked by framer/motion#1524 ) |
What?
Completely rewrite the
ToggleGroupControl's animated backdrop usingframer-motionand a couple of different approaches to reading size and position of DOM elements.Fixes #37506
Why?
Current implementation is complex, difficult to read, imperative (based off arbitrary timeouts), and buggy
How?
Main changes are:
framer-motionto handle the animationoffsetLeftinstead ofgetBoundingClientRect().leftin order to get the gap relative to the offset parent (instead of the viewport)useLayoutEffectin order to make sure that all DOM updates have taken effect before calculating sizes and offsetsTesting Instructions
Make sure that the backdrop animation works as expected in Storybook and in the editor (in particular testing for the scenario highlighted in #37506)
Screenshots or screencast
toggle-grou-control-animation.mp4
toggle-group-control-animation-editor.mp4