feat(tabs): animate background, current border#7336
Conversation
|
Preview: https://patternfly-pr-7336.surge.sh A11y report: https://patternfly-pr-7336-a11y.surge.sh |
|
This looks good to me. Is there ongoing work or good for review? |
There was a problem hiding this comment.
From a React perspective this sounds feasible. Looking into the existing code for Tabs stuff, I'm thinking we'd just leverage existing things for the most part:
- we're already using
componenDidUpdateto check if the children length changes (covers adding/removing tabs to recalculate the value of these vars) - We could use that ^ lifecycle to also check if the active tab changes and likewise recalculate the value of the vars
- We may need to add some logic within the Tab component to check if its content changes and recalculate the vars as well
Preventing the animation on initial render may be tricky depending if the animation would occur immediately, but as discussed if some modifier class were to be added to determine when to animate then we should be able to calculate the initial var values on the first render, then add whatever class to "enable" animations.
|
Do we need a separate issue for vertical tabs? |
|
I can see a little blue nubbin' on the tabs now and the blue underline seems to be thicker with this last update. Screen.Recording.2025-03-06.at.12.17.18.PM.mov |
|
I am also seeing the same nubbin' as well |
|
@andrew-ronaldson @kaylachumley sorry about that, it's not quite ready for review 😅 Aside from the nubbin' action, does the vertical tabs animation look as intended? |
Absofruitly! |
|
It looks like the old border is still showing here before the sliding one comes in on the examples from Tab Item Actions down. 2025-03-07_16-40-07.mp4 |
| } | ||
| } | ||
|
|
||
| .#{$tabs}.pf-m-no-animate-current { |
There was a problem hiding this comment.
I'm not sure about this name 😄 This turns off the slide on a case by case basis? Do we need this?
There was a problem hiding this comment.
For the time being, this is the class we'll add to tabs that keeps the animation from happening on initial render. Just for documentation purposes, here were the options from our slack thread about it
There are probably a few ways we can do that:
- React can keep the tabs from rendering until they set the vars for the current item, then render the tabs component. Eric said that might be tricky though.
- Core can add a class like .pf-m-accent-no-animate that disabled the animation, React can add that, set the vars, then remove the class. That's easy.
- I looked into another way of doing it. I think I can expose 2 sets of vars the react component could set - one set of vars animates and the other doesn't. React could set the vars that don't animate first, then on subsequent clicks, they set the vars that do animate
Feel free to suggest a better name! .pf-m-no-animate is too broad IMO since we may need a class like this for other animations, so I threw on "current", but I'm thinking .pf-m-no-animate-accent would be more appropriate. wdyt?
There was a problem hiding this comment.
I'm curious what scenario you can think of where we'd want to stop one animation but not others. My instinct is to use a more generic name like .pf-m-no-animation or .pf-m-pause-animation or even the .pf-m-no-motion that we already use.
There was a problem hiding this comment.
Most of our animations wouldn't need to be stopped like this because they either load with the initial component styles or after some sort of interaction or event. React needs to trigger the "current" animation logic manually (set the 2 vars for accent start and width) when the component mounts just to get the initial border style to render. None of our other animations require anything like that AFAIK.
That said, with a "no-motion" class (as in the user is opting out of the animation), if the goal is to not use the animation at all, I think that will need to do something different because if you simply disable the animation (duration set to 0 or whatever), you won't have an accent at all since the accent is created in a whole separate way depending on whether it's animated or not. That type of class will need to trigger the accent to be created on individual links instead of the list.
There was a problem hiding this comment.
Tried to think of what's bothering me about this one...
- the grammar 😛
- tokens use "motion" rather than "animate" to be more generic
- it's affecting accent variables so I think I'd prefer it to be named for that rather than current
- it's purpose is to freeze during load/initialization so I'm wondering if it could be named somehow for that state rather than no-animate
WDYT about something like pf-m-initializing-accent ?
|
The blue line has disappeared! Thanos snapped away! Ahhhhh |
|
@andrew-ronaldson yeah 2 things:
However, we won't be able to merge this PR in core before code freeze, because without the react work being done, the border would disappear for everyone. So I think we have some time to sort out how we should make the borders appear in core, or if we leave them off (similar to how we don't show expanded menus in core since that's react functionality) |
|
Could we have a selected tab in the examples to show the states or is it entirely in react? |
There was a problem hiding this comment.
But I liked "tubtab" 😉
Also, wasn't sure if you were ready to remove the commented out JS or not.
| {{/if}} | ||
| } | ||
| )()" | ||
| --}} |
There was a problem hiding this comment.
did you want to remove this yet?
| border-inline-start-width: var(--#{$tabs}--link-a 4268 ccent--BorderInlineStartWidth); | ||
| transition: | ||
| --#{$tabs}--link-accent--length var(--#{$tabs}--link-accent--TransitionDuration--length) var(--#{$tabs}--link-accent--TransitionTimingFunction--length), | ||
| --#{$tabs}--link-accent--start var(--#{$tabs}--link-accent--TransitionDuration--start) var(--#{$tabs}--link-accent--TransitionTimingFunction--start), |
There was a problem hiding this comment.
| --#{$tabs}--link-accent--start var(--#{$tabs}--link-accent--TransitionDuration--start) var(--#{$tabs}--link-accent--TransitionTimingFunction--start), | |
| --#{$tabs}--link-accent--start var(--#{$tabs}--link-accent--TransitionDuration--start) var(--#{$tabs}--link-accent--TransitionTimingFunction--start) |
There was a problem hiding this comment.
I'm wondering about breaking this up rather than using the shorthand. It is a little harder to make sure the timings are lined up - but also, do we need to have separate timing function and duration variables, or could we have one for the entire effect - that we'd always want them to be the same since it's one motion.
| text-decoration-line: none; | ||
| background-color: var(--#{$tabs}__link--BackgroundColor); | ||
| border-radius: var(--#{$tabs}__link--BorderRadius); | ||
| transition: background-color var(--#{$tabs}__link--TransitionDuration--background-color) var(--#{$tabs}__link--TransitionTimingFunction--background-color); |
There was a problem hiding this comment.
Do you want to separate this rather than using the shorthand?
There was a problem hiding this comment.
Doh! Yep - fixed.
Mar-12-2025.09-09-01.mov
| --#{$tabs}--link-accent--TransitionDuration--length: 0; | ||
| --#{$tabs}--link-accent--TransitionDuration--start: 0; |
There was a problem hiding this comment.
Should these two lines now just be --#{$tabs}--link-accent--TransitionDuration: 0;?
There was a problem hiding this comment.
oops, missed this PR! I went to review but the animated tabs isnt working for me... maybe user error. I tried a new browser and nada! is the animated tabs working for you? @mcoker
|
@kaylachumley since the border depends on javascript I have to manually enable it in this PR, but will want to take it out prior to merging this PR since we don't include javascript like this in the core examples. I've re-enabled it so you can see it on all of the examples now - https://patternfly-pr-7336.surge.sh/components/tabs |
|
looks awesome |
|
🎉 This PR is included in version 6.3.0-prerelease.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #7250