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(text): removes always computing text element alignment #580

Closed
wants to merge 1 commit into from

Conversation

wkashdan
Copy link
Contributor

Describe the problem this PR addresses

The text element calls getComputedStyle which is very inefficient sometimes causing many milliseconds of lag and layout recalculations.

Describe the changes in this PR

This PR removes calculating the computed text alignment. This means the padding fix will only be set when text alignment is directly set on the element and has letter spacing (whether inherited or not).

@wkashdan wkashdan requested a review from a team as a code owner March 26, 2024 21:50
Copy link

Deployed Styleguide and Lab.

Notes
  1. Links may take a few minutes to update after PR is opened or commit is pushed.
  2. Links may become invalidated after PR is merged or closed.
  3. Links for all releases and open PRs can be found on the Maker Deploys page.

@privatenumber
Copy link
Member

Thanks @wkashdan
Do you mind adding the screenshot and the context you provided me over DM?
Specifically, I think you mentioned this happens on scroll, which is unexpected.


The fix you're removing (#561) corrects text alignment in the Editor and Published sites so I'm not sure if we can remove it.

I think the fix is actually fine because it only runs on mounted and updated. I think this begs the question: why is the Vue subtree getting "updated" on scroll? I think even if we remove the getComputedStyle here, unless we prevent the "updates" happening on scroll, we'll always have unexpected perf issues.

@wkashdan
Copy link
Contributor Author

Closing, as @privatenumber recommended not going with this approach

@wkashdan wkashdan closed this Mar 27, 2024
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.

None yet

2 participants