Changed toHaveStyle to use getPropertyValue instead of accessing the property directly #285
Conversation
…property directly
| <span data-testid="color-example" style="font-size: 12rem">Hello World</span> | ||
| `) | ||
| expect(queryByTestId('color-example')).toHaveStyle({ | ||
| fontSize: 12 |
There was a problem hiding this comment.
Why would this match 12rem? If I set font-size: 12; then this results in 12px. This seems a bit confusing to me.
Also: This test only works in JSDOM. In an actual browser the value would be resolved to (depending on the base font size) to 240px.
There was a problem hiding this comment.
You are right! This test case doesn't make any sense as it is going to work only in JSDOM. Btw, I didn't know that, thanks! I'm going to change that.
It isn't clear to me if you are only against this test case or the idea of the api.
There was a problem hiding this comment.
Just want to make sure that we're not documenting a test case that wouldn't work in a correct DOM environment.
There was a problem hiding this comment.
Follow up on this. It seems from the discussion above that there's still something to be addressed here before this is ready. Right?
There was a problem hiding this comment.
Is JSDOM actually returning 12rem here has the computed style? Then all we have to do is add a comment explaining that this test depends on the user agent (since it will return the computed font-size in pixel which depends on the base font size for rem).
There was a problem hiding this comment.
I changed to px right after your first comment, do you think that is still worth to leave a note about that behavior?
There was a problem hiding this comment.
@luanraithz thanks a lot for this.
I left a few comments that may need your attention. But overall it looks good.
| <span data-testid="color-example" style="font-size: 12rem">Hello World</span> | ||
| `) | ||
| expect(queryByTestId('color-example')).toHaveStyle({ | ||
| fontSize: 12 |
There was a problem hiding this comment.
Follow up on this. It seems from the discussion above that there's still something to be addressed here before this is ready. Right?
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #285 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 315 315
Branches 72 72
=========================================
Hits 315 315
Continue to review full report at Codecov.
|
|
@luanraithz I think this is almost ready to merge, but I wanted your input on two things before we do:
|
|
|
🎉 This PR is included in version 5.11.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
Fixes #280
Fixes #281
Why:
As #280, is a regression problem, which wasn't covered by tests.
How:
As mentioned by @just-boris, just changed to use the function
getPropertyValueinstead of accessing the property directly.Checklist: