search

square / maker

Maker Design System by Square
https://square.github.io/maker/styleguide/latest-stable/
Other
63 stars 14 forks source link

fix(text): removes always computing text element alignment #580

Closed wkashdan closed 6 months ago

wkashdan commented 6 months ago

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).

github-actions[bot] commented 6 months ago

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 commented 6 months ago

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 (https://github.com/square/maker/pull/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 commented 6 months ago

Closing, as @privatenumber recommended not going with this approach