fix: support uppercase custom props in toHaveStyle #552
Conversation
CSS custom properties are case sensitive fixes testing-library#551 --- this commit adds a failing test, the following one makes it pass please squash them together
please combine me with the previous commit using "squash+merge"
src/to-have-style.js
Outdated
| computedStyle.getPropertyValue(prop.toLowerCase()) === value, | ||
| computedStyle.getPropertyValue(prop) === value, |
There was a problem hiding this comment.
Wait, I think we may have a problem. I suspect that it was done this way because non-custom properties are case-insensitive. At least a quick check in the browser shows that setting Display: none hides the element.
Can you make it so that we keep the lower case transformation when the prop does not look like a custom css prop?
There was a problem hiding this comment.
Interesting, I'll look into it tomorrow!
There was a problem hiding this comment.
Good that there was a test case added with #148. Could have found out via git blame.
| @@ -214,7 +214,7 @@ describe('.toHaveStyle', () => { | |||
| <span data-testid="color-example" style="font-size: 12rem">Hello World</span> | |||
| `) | |||
| expect(() => { | |||
| expect(queryByTestId('color-example')).toHaveStyle({ fontSize: '12px' }) | |||
| expect(queryByTestId('color-example')).toHaveStyle({fontSize: '12px'}) | |||
There was a problem hiding this comment.
That one came from the husky pre-commit hook formatting the whole file. By the way running npm run format would have changed a bunch of other files, too.
| @@ -205,7 +205,7 @@ describe('.toHaveStyle', () => { | |||
| <span data-testid="color-example" style="font-size: 12px">Hello World</span> | |||
| `) | |||
| expect(queryByTestId('color-example')).toHaveStyle({ | |||
| fontSize: 12 | |||
| fontSize: 12, | |||
src/to-have-style.js
Outdated
| computedStyle[prop] === value || | ||
| computedStyle.getPropertyValue(prop) === value || | ||
| computedStyle.getPropertyValue(prop.toLowerCase()) === value, |
There was a problem hiding this comment.
Can you make it so that we keep the lower case transformation when the prop does not look like a custom css prop?
After reading the whole discussion here, I thought this would be the least disruptive way.
There was a problem hiding this comment.
I still think that we may need to detect specifically if the prop name corresponds to a custom CSS property. They are case-insensitive. The way the code is right now, if I have an element with --var-name: 0px, a test checking for --VAR-NAME: 0px would pass, and that's not accurate.
Can't you do something along the lines of:
const isCustomProp = prop.startsWith('--')
if (!isCustomProp) {
prop = prop.toLowerCase()
}
// Keep the condition as beforeThere was a problem hiding this comment.
see my latest commit
src/to-have-style.js
Outdated
| computedStyle[prop] === value || | ||
| computedStyle.getPropertyValue(prop) === value || | ||
| computedStyle.getPropertyValue(prop.toLowerCase()) === value, |
There was a problem hiding this comment.
I still think that we may need to detect specifically if the prop name corresponds to a custom CSS property. They are case-insensitive. The way the code is right now, if I have an element with --var-name: 0px, a test checking for --VAR-NAME: 0px would pass, and that's not accurate.
Can't you do something along the lines of:
const isCustomProp = prop.startsWith('--')
if (!isCustomProp) {
prop = prop.toLowerCase()
}
// Keep the condition as before
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 27 27
Lines 664 668 +4
Branches 251 252 +1
=========================================
+ Hits 664 668 +4 ☔ View full report in Codecov by Sentry. |
|
🎉 This PR is included in version 6.1.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
support uppercase custom props in toHaveStyle
fixes #551
Why:
CSS custom properties are case sensitive
How:
don't lowercase custom property names before comparing them
Checklist:
--
please squash and merge
related: #148, #164