8000 fix(tabs): add opt-in for current animation, performance by mcoker · Pull Request #7503 · patternfly/patternfly · GitHub
[go: up one dir, main page]

Skip to content

fix(tabs): add opt-in for current animation, performance#7503

Merged
mcoker merged 8 commits intopatternfly:mainfrom
mcoker:issue-7437
May 20, 2025
Merged

fix(tabs): add opt-in for current animation, performance#7503
mcoker merged 8 commits intopatternfly:mainfrom
mcoker:issue-7437

Conversation

@mcoker
Copy link
Contributor
@mcoker mcoker commented May 1, 2025

fixes #7437

Moves animation behind .pf-m-animate-current. Converts width/inset-inline-start transition to scale/translate for supercharged turbo performance TO THE MAX!!!

@patternfly-build
Copy link
< 8000 div class="d-none d-sm-flex"> Collaborator
patternfly-build commented May 1, 2025

Copy link
Contributor
@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good pulling these latest changes into React -- animated tabs animate and the HTML/class implementation used from Org in the local build looks how it should without animations

Copy link
Member
@srambach srambach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine but FWIW, the animated tabs do get messed up in RTL, so if that's not easy to fix, maybe we should remove the inline JS - if so, I'd add a comment on the example clearly saying that you won't see the effect here and link to the React example showing it.

Copy link
Contributor
@sg00dwin sg00dwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcoker
Copy link
Contributor Author
mcoker commented May 13, 2025

@srambach good catch with RTL. Since this uses translate now, I needed to change the translate to a negative number in RTL. You can test by clicking on something, then switching to RTL and looking at the computed value.

Also updated the JS to get the "right" value by calcing the container width - current item left - current item width. @thatblindgeye is that what the RTL util you added does?

I'd still like to remove this JS once this PR is ready, or as a follow-up.

@thatblindgeye
Copy link
Contributor

The util is basically just taking the 8000 offsetLeft and adding it to the offsetWidth of the pf-m-current tab, and subtracting that from the offsetWidth of the outermost tabs list container.

@mcoker mcoker requested a review from srambach May 19, 2025 18:39
Copy link
Member
@srambach srambach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L🐈TM (with js removed)
I wouldn't hate mentioning which classes and variables apply only to the animated version in the component docs.

@mcoker mcoker merged commit a580ea0 into patternfly:main May 20, 2025
2 of 4 checks passed
@mcoker mcoker deleted the issue-7437 branch May 20, 2025 20:27
@patternfly-build
Copy link
Collaborator

🎉 This PR is included in version 6.3.0-prerelease.19 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tabs - animation and core styles, performance

5 participants

0