-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Tabs: indicator animation #60560
Tabs: indicator animation #60560
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason why we're not using shared layout animations for the indicator?
We are already using them for ToggleGroupControl
, see #50278.
@tyxla that is a nice API! I didn't know about the use of Framer Motion in this package. I'm hesitant about adding 40.2kb (gzipped) to the bundle size, especially for "nice to have" effects like these. I think it is simple enough to be implemented with ResizeObserver + CSS in what probably amounts to less than half a kilobyte (that's about 80 times smaller than the I'm curious, is there an argument for using it, other than the fact we're already using it somewhere else? I think this is something that could be worth simplifying in the long run, shaving ~40kb off the bundle seems like a huge win to me! Related thought - the animation that you linked seems very similar to this one, so I wonder if a very small utility could be extracted from my code here and used for both. That'd get the bundle size down even further while keeping things simple. |
Yes, we're mindful of bundle size, however, the
Now that we're ending up doing the same kind of shared animation multiple times, abstracting it could indeed be a good idea for a follow-up. |
That's fair, though one could then argue that it adds to the technical debt that will make transitioning away from it harder in the future. Right now the argument is "we already use X so might as well use it here", which is totally reasonable, but has the effect that in the future, when the idea of phasing "X" out is proposed, the argument "we use X in many places so it's an expensive refactor we can't afford" will have more weight. I don't know where the team philosophy sits in regards to this. I've taken a look around all uses of |
This is always the case for refactoring, though, so I'll disagree with you, since it's not a strong argument for me. It is a marathon, not a sprint.
I'm happy to remove all dependencies, of course. However, I'd advocate making small, step-by-step improvements. Adding the animation with whatever we're already doing/using feels the most straightforward to me. Then, as a next step, if we want to refactor away from Let me know what you think. |
Update: I refactored this to use the Framer Motion lib, and unfortunately there are a ton of problems when testing the component instances in a wp instance (you can load this PR in the playground to see them). I've spent quite a bit of time today trying to find a decent solution but nothing's working, so I might take a small break from this PR, especially since I'm working on adding support for loading specific Gutenberg commits to the playground - a feature that will be very helpful in debugging this. |
I'd love to learn more about the problems you encountered. I gave this some testing in a bunch of places, but the only weird behavior I stumbled upon were:
Screen.Recording.2024-04-18.at.22.22.36.mov
Screen.Recording.2024-04-18.at.22.34.03.movBoth of these seem fixable, although I haven't had a chance to play with them. Are there any other problems you encountered? |
Thanks for looking into it @tyxla! 1 is just a TODO, but 2 is indeed one of the issues. For that one, I've tried to debug it in multiple ways, but always assumed that the The other issue I've noticed is that there's an annoying animation on page load on some instances of Tabs, likely because there is some sort of immediate re-render that Framer Motion is picking up and animating. You can observe it here, I'm constantly reloading the page in this recording: Kapture.2024-04-19.at.12.24.51.mp4I also spent a good amount of time trying to figure out a "clean" universal solution that isn't a dirty hack, but without control of the animation it's a bit hard to come up with something other than a hacky initial If you have any ideas, I'd be happy to hear them, as I feel a bit stuck! |
That's a correct assumption, since that's how the tabs context is implemented, but it's worth verifying case-by-case that it's actually true. Maybe we're inadvertently reusing the same instances in multiple places. An alternative in the case of duplication is to use wrapping
Oh, that one. That occurs because during initial load, the first tab name is called "Document":
And once the current post type data loads, it changes the label from "Document" to the name of the post type ("Post" in your example). Because "Document" is longer than "Post", you'll see |
I verified that this is not the case, instance IDs for all rendered tabs when reproducing these issues are unique. What do you mean by "reusing the same instances"? I haven't looked at the source, but my understanding is that these IDs are unique to the React element, is this correct?
Yes, I understand why this happens. While it is "correct", I don't think it's what we want, I believe what we want is to animate the indicator when transitioning between different tabs. I've been looking at Framer Motion and haven't been able to find any granular way to control (i.e. enable/disable) the animation. I know how I could do it on my previous implementation though, and it is pretty straightforward, so it's a shame that Framer Motion doesn't support this. Maybe I just missed it, I was actually surprised when I couldn't find it. By the way, I'm not sure why because the context and code is exactly as it was when I last worked on this, but now I'm seeing a new buggy behavior: Kapture.2024-04-22.at.11.52.44.mp4It doesn't jump from the opposite direction like previously, but now it jumps in a glitchy, incorrect way. This one's a real head-scratcher :( |
Another one: Kapture.2024-04-22.at.12.04.44.mp4 |
@tyxla check the new vertical tabs story to see the current look. I think there's a point to reverting it to how it worked before (horizontal and no animation), but take a look first just so we're on the same page :) |
I've done some extensive testing in the post editor, site editor and storybook and things look great, aside from the RTL support (see below). Code also looks good - thanks for taking the time to explain some of the decisions!
I think it works great, thanks! The only thing I can see not working well is RTL support - right now the indicator will appear on the right for RTL, which is undesired: RTL: Also, can we get a pair of eyes from @WordPress/gutenberg-design before we ship it? |
Nice work. The horizontal animation where the bar moves to the tab that gets selected, that works great, as it echoes the segmented control backdrop animation: It should probably also use the same animation metrics, speed, bounce, etc — the idea being, these are the same physics, just a different visual. I think @youknowriad explored an alternative to framer motion recently, would be good to double check first. I would not add the line to these vertical tabs: If we add animation to the vertical tabs, it should be the gray backdrop itself that moves, just like the backdrop on the segmented control moves. |
@jasmussen thanks for chiming in!
Agreed. Since I'm gonna be re-using some of the logic to rework some of those components as a follow-up, I will make sure then that it uses the same animation parameters 👍 Re: Framer Motion, this uses a vanilla CSS transition, with only some minimal logic to track position and size of the indicator and some tweaks to prevent unintended animations. The goal is to achieve the same, potentially with some code re-use, in those other components as well. Related: #60975
I think that's a great idea. No RTL concerns either with that approach cc @tyxla. I'm gonna experiment with it and ping back for feedback. |
Sounds good to me as well. One way to approach it is to leave vertical tabs without animation, and address it in a separate PR, to keep changes as contained as possible. |
In terms of the animation itself, I think we can play with a different easing, maybe add a small bit of bounce (a variation of The goal is that the tabs backdrop will use the same CSS animation instead of framer motion, FWIW. Framer animations can be beautiful, but are very heavy on resources and come with too many trade-offs, thus our goal to minimize their use and avoid using them for the components package. Ideally the View Transitions API will get a broader support and we'll be able to completely remove the package in favor of those rich, slick, optimized transitions. None of this needs to happen in this PR. I think this is already quite spot on and we can go ahead with it once we solve (by removing for example) the vertical tab animation. |
@tyxla feel free to suggest a specific easing curve, something like this might help, and you can also use the Chrome DevTools to see it in action directly in the storybook: Kapture.2024-05-23.at.11.56.46.mp4I still need to push a couple of remaining tweaks here anyway and it's a one line change, so there's room for one last feedback round :) |
Regarding the view transitions API, it is indeed very exciting though in my experience using it, it has some limitations and can be problematic for micro-interactions. The simplicity of it is also the source of these limitations, interestingly. Last I remember, the way it fundamentally works is by taking a screenshot of the DOM content before and after the transition, and then transitioning select parts of it by literally moving pixels around. While this simplifies certain transitions, especially between pages, it also means that some other concerns like interactivity in other parts of the page conflict with the transition. In this case, implementing the animation of the tabs indicator with this API might result in preventing interaction with the rest of page during its course, or at least result in a broken animation if interrupted, while the current approach is local to the Tabs DOM and resilient to anything that might be going on elsewhere in the page, and even to transformations (thanks to my use of offset- parameters for position and size tracking, and relative/local variable values in CSS). I still think it is worth experimenting with, and maybe I'm wrong and the API has changed enough to support these use-cases properly (I would be delighted if that's the case), but we need to remember that the main purpose of this API is for page-wide layout animations rather than micro-interactions, and important trade-offs exist. |
I'd keep it something simple (without an obvious bounce), like https://easings.net/#easeInOutCubic or https://easings.net/#easeInOutExpo but I have no strong preferences. I'm definitely not an easing expert, so feel free to experiment with different ones. Again, this is something we can do subsequently in another PR. |
@tyxla sure! I'm personally a fan of Would love your thoughts here too @jasmussen. None of this is blocking for this PR btw, just want to get the convo going. |
My instinct also veers towards ease-out, though a CC to @jameskoster as well, as he's also been diving into this. My main recommendation for the animations in general is that they should feel fluid and snappy. Fast. Almost never longer than 0.2 seconds, sometimes 0.1s in speed. The best way to feel this out, is in the branch itself and so far the speed seems fine — and if nothing else, the speed from the segmented control is the model to follow. I'm also assuming you are already on top of this (and I think it's built-in to the componentry), but will state it to just be sure, all of these animations should respect a users |
It would probably make sense to tokenise animation effects at some point, we're a bit all over the place at the moment.
For now I agree it makes sense to mimic the segmented control 1:1, then revisit holistically. |
Thanks everyone. I've pushed a couple of last details and this should be ready to merge. Feel free to do a final review @tyxla, looking at my latest commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and tests well! 🚀
Thanks @DaniGuardiola! 🙌
Let's follow-up on adding the animation to the vertical tabs when you get a chance.
@richtabor I will take a look at it, thanks for reporting. Edit: #61973 |
* Initial tab indicator animation implementation * Add changelog entry * Minor tweak * Fix downstream issues. * Use ResizeObserver. * Add width transition. * Simplify and use framer motion * vertical indicator * Revert to previous implementation. * Fix bug due to some animations breaking measurement of the tab element. * Abstracted and fixed all previous issues. * Follow naming convention for classes. * Support vertical orientation + misc fixes and improvements. * Clean up styles a bit. * Better focus ring animation + minor style cleanup. * Fix changelog (oops). * Actually fix changelog. * Remove deprecated `reduceMotion` utility. * Fix open/closed * Add vertical tabs story * Move ResizeObserver unobserve to effect cleanup * Remove outdated type cast. * Hide vertical indicator for now. Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
👋 Catching up with this conversation, I'd be interested in debugging a bit more the issues encountered while using framer motion, as I debugged a few similar ones while working on One of the main reasons we opted for using Also, since framer motion is exported from the components package, I'm not sure it's going to be possible to remove it entirely as a dependency. In general, I agree that both for visual consistency and for long-term maintainability, we should aim at having a unified way of implementing such animations across our components. |
* Initial tab indicator animation implementation * Add changelog entry * Minor tweak * Fix downstream issues. * Use ResizeObserver. * Add width transition. * Simplify and use framer motion * vertical indicator * Revert to previous implementation. * Fix bug due to some animations breaking measurement of the tab element. * Abstracted and fixed all previous issues. * Follow naming convention for classes. * Support vertical orientation + misc fixes and improvements. * Clean up styles a bit. * Better focus ring animation + minor style cleanup. * Fix changelog (oops). * Actually fix changelog. * Remove deprecated `reduceMotion` utility. * Fix open/closed * Add vertical tabs story * Move ResizeObserver unobserve to effect cleanup * Remove outdated type cast. * Hide vertical indicator for now. Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
@jasmussen I added the vertical indicator animation: #62879 |
What?
Makes the
Tabs
indicator animated (except when the user prefers reduced motion).Why?
Looks cool. @mirka can probably add more context :)
How?
See styles. Position and length is synced through the Ariakit state store.
Testing Instructions
Play with it on Storybook. Optionally simulate the "prefers reduced motion" setting through devtools.
Testing Instructions for Keyboard
Tab into the component, then use arrow keys to change tabs.
Screenshots or screencast
Kapture.2024-04-08.at.12.14.27.mp4
In Windows high-contrast mode:
Kapture.2024-04-08.at.12.40.31.mp4