ProgressBar: moved width to css var for perf #60388

DaniGuardiola · 2024-04-02T16:19:00Z

What?

Moved width styles for the bar (depends on value prop) out of emotion for performance and replaced with CSS variable.

Why?

Dynamic changes in emotion (CSS-in-JS) are slow.

How?

As described.

Testing Instructions

Manually in Storybook (ProgressBar component, change value. There is a unit test that checks the rendered CSS too.

Testing Instructions for Keyboard

n/a

Screenshots or screencast

n/a

github-actions · 2024-04-02T16:19:15Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-04-02T16:19:25Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @DaniGuardiola! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

tyxla

Thank you for working on this and congrats on your first Gutenberg PR 🙌

I wonder if there's a good reason to not pass width through the style prop directly?

I was also curious if you were able to observe any measurable performance improvements.

Finally, it may be a good idea to add a CHANGELOG entry, since it's a neat little enhancement.

packages/components/src/progress-bar/index.tsx

packages/components/src/progress-bar/styles.ts

tyxla · 2024-04-03T09:48:57Z

packages/components/src/progress-bar/test/index.tsx

@@ -56,7 +56,7 @@ describe( 'ProgressBar', () => {
 		const indicator = container.firstChild?.firstChild;

 		expect( indicator ).toHaveStyle( {
-			width: '55%',
+			'--indicator-width': '55%',


We will be able to revert this if we use the prop directly.

I'd add that I don't think this test should exist, as it's testing implementation details and not necessarily observable behavior. Instead, this should be tested through visual snapshot testing or similar IMO.

Hm, I'd disagree this is testing an implementation detail. The definition by Kent C. Dodds says:

Implementation details are things which users of your code will not typically use, see, or even know about.

I'd argue this isn't the case for the width of the progress bar indicator - the indicator is actually defined by its width for the user, and it's definitely the primary thing the user will see from that component.

That being said, I still think a variation of this test is necessary, but it doesn't necessarily have to test a CSS var, but rather, the width of the element for example.

The width is resolved by the web rendering engine, which jsdom doesn't have, so it's not possible to test this in the context of a unit test. Leaving the (potentially very interesting!) philosophical debate about what qualifies as an implementation detail, in this specific case I'd highlight the fact that what the test is doing (in the previous and current versions) is test React/emotion for the most part, and whether when you set a value, the value is set. For that alone I don't see much value.

Regarding implementation details (and with no intention to prolong this more than necessary, just for fun :P), I'd argue that what's observable here is what the user sees or whether pixels are rendered a certain way. My understanding of testing observable behavior is that, if you change the implementation, the test doesn't need to be modified. In this case, if we were to switch to an SVG path based implementation, or maybe a CSS transform one, we'd need to change the test since it directly depends on the way it is implemented.

You can definitely test the observable part here though, through visual snapshot testing, or even with a headless browser that does implement a rendering engine (cypress, puppeteer, etc).

I think we're more or less on the same page about what an implementation detail is, but it's also about compromise in favor of keeping as few e2e tests as possible, because they come at their own added cost. Is an e2e test more useful in this case, being less about the actual implementation and more about what the user sees - absolutely. Is it more expensive to have and maintain - yes. Thus the compromise.

On another note, visual regression testing is definitely something we've been aiming to have for some scenarios, and it can be handy as we iterate on components. I know @mirka has been experimenting with that before.

While testing styles in Jest are indeed a lower-cost compromise over a full vizreg test, it also isn't cost-free in the sense that it can add a maintenance cost that can be arguably disproportionate to the low confidence that it adds — similar to a snapshot test.

In this particular case I don't think it's likely that someone would break the style and land that PR without anyone noticing (at least for the happy path), so to be honest I don't care that much about whether or how we do the test. If it does ever break, we can perhaps alter the strategy based on how it broke.

DaniGuardiola · 2024-04-03T10:33:52Z

@tyxla

Thank you for working on this and congrats on your first Gutenberg PR 🙌

Thank you! :)

I wonder if there's a good reason to not pass width through the style prop directly?

Yes, a few:

Keeping all styles in the same place.
Inline styles take precedence over almost anything, which greatly hurts extensibility / overridability.
Injecting JS-derived values as variables and then using those variables from CSS is an easier mental frame than mixing JS with styles directly.
Having the variable available like this enables future changes in a very easy way.

E.g. imagine we switch to an svg stroke here, which might actually make sense since animating that is more performant than animating width - then we'd need to do some calc()ulations to turn the percentage into the right value for stroke-dasharray or stroke-offset, and this can be achieved without changing any of the JavaScript logic. It's a useful separation that becomes more and more useful the more consistently that it is applied.

Ariakit does this for most of the public elements that it renders. Since these components are an extension of Ariakit components, there's also a point about having a consistent pattern.

All of this said, I'd like to know if you have any arguments for using inline styles for this (other than it being slightly shorter, which is the only one I can think of at the moment)?

I was also curious if you were able to observe any measurable performance improvements.

I wondered about this too and tried to manually test it, but it's not really noticeable in the Storybook. I was pairing with Lena and she told me that this kind of dynamic emotion changes are not performant in general, which makes a lot of sense to me.

Finally, it may be a good idea to add a CHANGELOG entry, since it's a neat little enhancement.

That was my last commit yesterday, but I forgot to push it :P

packages/components/CHANGELOG.md

tyxla · 2024-04-03T10:55:00Z

packages/components/src/progress-bar/styles.ts

@@ -70,7 +69,7 @@ export const Indicator = styled.div< {
 					width: `${ INDETERMINATE_TRACK_WIDTH }%`,
 			  } )
 			: css( {
-					width: `${ value }%`,
+					width: 'var(--indicator-width)',


Do we need to prefix this var somehow so it doesn't accidentally bump into another one? Sometimes we use --wp-admin, sometimes we use --wp-components, and sometimes --wp, so perhaps it can be something like --wp-progressbar-indicator-width or something? @mirka thoughts?

We discussed this during our pairing session and settled on this shorter one. I think it's very unlikely to collide with other css vars, and it's internal to a component that doesn't have any slots either. The cool thing about CSS vars is that they are local to the element tree where they are declared, so collisions are almost never a concern unless the tree contains external, user-provided elements.

Sounds good - let's keep it as-is 👍

tyxla · 2024-04-03T10:56:06Z

Thanks for elaborating @DaniGuardiola! I think that rationale is in total favor of keeping the CSS var 👍

tyxla

This looks good 👍 🚀 Thanks @DaniGuardiola!

Perhaps we could update the test to actually test the width and not the CSS var, and rename the CSS var to be less prone to naming conflicts, but both of those can be done as follow-ups if you agree.

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

tyxla

LGTM 🚀

Nice work with your first Gutenberg PR @DaniGuardiola 👏

mirka · 2024-04-03T13:42:08Z

For context, I'll add that this change was not motivated by any actual measured performance issue, but more to adhere to the best practices recommended by Emotion itself (Use the style prop for dynamic styles and CSS variables with style). These changes will also help us move off of Emotion, as they are closer to more standard ways of styling.

* ProgressBar: moved width to css var for perf * fixed unit test * added changelog entry * remove accidentally pushed test code * Update packages/components/CHANGELOG.md Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> --------- Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org>

DaniGuardiola added 2 commits April 2, 2024 17:50

ProgressBar: moved width to css var for perf

b4b88bc

fixed unit test

853cf29

DaniGuardiola requested a review from ajitbohra as a code owner April 2, 2024 16:19

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Apr 2, 2024

added changelog entry

8e24222

mirka assigned DaniGuardiola Apr 2, 2024

mirka requested a review from a team April 2, 2024 16:51

mirka added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Apr 2, 2024

tyxla reviewed Apr 3, 2024

View reviewed changes

remove accidentally pushed test code

85de0c1

Merge branch 'trunk' into fix-progress-bar-perf

ac316ab

DaniGuardiola requested a review from tyxla April 3, 2024 10:41

tyxla reviewed Apr 3, 2024

View reviewed changes

tyxla approved these changes Apr 3, 2024

View reviewed changes

Update packages/components/CHANGELOG.md

f217b8e

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

DaniGuardiola requested a review from tyxla April 3, 2024 12:20

tyxla approved these changes Apr 3, 2024

View reviewed changes

tyxla merged commit 2d33a9e into WordPress:trunk Apr 3, 2024
59 of 60 checks passed

github-actions bot added this to the Gutenberg 18.2 milestone Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProgressBar: moved width to css var for perf #60388

ProgressBar: moved width to css var for perf #60388

DaniGuardiola commented Apr 2, 2024

github-actions bot commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024

tyxla left a comment

tyxla Apr 3, 2024

DaniGuardiola Apr 3, 2024

DaniGuardiola Apr 3, 2024

tyxla Apr 3, 2024

DaniGuardiola Apr 3, 2024

tyxla Apr 3, 2024

mirka Apr 3, 2024

DaniGuardiola commented Apr 3, 2024

tyxla Apr 3, 2024

DaniGuardiola Apr 3, 2024 •

edited

Loading

tyxla Apr 3, 2024

tyxla commented Apr 3, 2024

tyxla left a comment

tyxla left a comment

mirka commented Apr 3, 2024

ProgressBar: moved width to css var for perf #60388

ProgressBar: moved width to css var for perf #60388

Conversation

DaniGuardiola commented Apr 2, 2024

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Apr 2, 2024 • edited Loading

github-actions bot commented Apr 2, 2024

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniGuardiola commented Apr 3, 2024

Choose a reason for hiding this comment

DaniGuardiola Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Apr 3, 2024

tyxla left a comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

mirka commented Apr 3, 2024

github-actions bot commented Apr 2, 2024 •

edited

Loading

DaniGuardiola Apr 3, 2024 •

edited

Loading