-
Notifications
You must be signed in to change notification settings - Fork 392
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I'll look into it tomorrow!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
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.
same here
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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