Skip to content
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

fix(lemon-ui): Stop infinite re-renders of LemonTabs #18645

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Nov 15, 2023

Problem

LemonTabs LemonTabs--transitioning is firing non-stop
2023-11-14 18 59 21

Changes

Remove the prop from the hook

How did you test this code?

Clicked around and didn't seem to impact re-renders

@Twixes Twixes changed the title fix: stop transitioning over firing causing re-renders fix(lemon-ui): Stop infinite re-renders of LemonTabs Nov 15, 2023
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

3 snapshot changes in total. 0 added, 3 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes
Copy link
Member

Twixes commented Nov 15, 2023

This is what this was supposed to fix:

Screenshot 2023-11-15 at 12 41 35

LemonSegmentedButton off completely – but only in the support modal, and only on initial render. I couldn't reproduce this anywhere else, and had no idea what's going on. Only the re-render solved this. But obviously I missed that this is really bad for LemonTabs, as that uses the transitioning value properly, and LemonSegmentedButton didn't.

And naturally, in this PR the problem is back.

I looked into it deeper now and finally realized what was going on – .getBoundingClientRect(), that the hook was using, takes transform: scale(x) into account, and so the calculations were only correct once the modal appearance transition was over. That was the whole problem.

So I pushed a fix to switch to offsetLeft/offsetWidth, which solves that issue, plus added a .LemonSegmentedButton-transitioning class to animate the sliding properly.

@Twixes Twixes enabled auto-merge (squash) November 15, 2023 12:31
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 0 modified, 0 deleted
  • webkit: 0 added, 1 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes merged commit 45c9807 into master Nov 15, 2023
70 checks passed
@Twixes Twixes deleted the dn-fix/transitioning-firing branch November 15, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants