-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
ProgressBar: moved width to css var for perf #60388
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -56,7 +56,7 @@ describe( 'ProgressBar', () => { | |||
const indicator = container.firstChild?.firstChild; | |||
|
|||
expect( indicator ).toHaveStyle( { | |||
width: '55%', | |||
'--indicator-width': '55%', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will be able to revert this if we use the prop directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thank you! :)
Yes, a few:
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 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 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.
That was my last commit yesterday, but I forgot to push it :P |
@@ -70,7 +69,7 @@ export const Indicator = styled.div< { | |||
width: `${ INDETERMINATE_TRACK_WIDTH }%`, | |||
} ) | |||
: css( { | |||
width: `${ value }%`, | |||
width: 'var(--indicator-width)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - let's keep it as-is 👍
Thanks for elaborating @DaniGuardiola! I think that rationale is in total favor of keeping the CSS var 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Nice work with your first Gutenberg PR @DaniGuardiola 👏
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 |
* 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>
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