-
Notifications
You must be signed in to change notification settings - Fork 406
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
toHaveStyle doesn't take account of CSS custom properties #280
Comments
Indeed, #276 was there for a reason, and unfortunately, we had not been covering in a test the support for these custom properties, which would've flagged this when it was introduced. The reason for #276, you can see it by checking #272, which is pretty serious and un-intuitive. Anyway, we need to figure out how to support both use-cases. I may be able to dig into this soon, but I'll also mention here @just-boris who flagged #272 and authored the fix, because maybe they have an immediate understanding of what could be needed here. |
I see two possible options here:
|
Option (1) seems reasonable enough without the extra overhead of the case converter. |
BTW @thomlom in the mean time, while this is resolved, you need not have a fork. You can fix the version in your package.json to the one prior to the release of that recent fix. |
Option 1 seems good to me too! Tests with a CSS custom property would need to be added of course. @gnapse, sure! I didn't mean "forked" but "cloned" 😄 |
🎉 This issue has been resolved in version 5.11.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
@gnapse can we please get this issue reopened as it doesn't appear to have been resolved. |
If the fix lies on jsdom, then the fix on our side is to merely update the jsdom dependency, right? Which, BTW, may require an update to the jsdom dependency in dom testing library. Anyway, I'll open this for now to reflect this situation. |
Any update here? Thanks! |
Not yet. I wonder if updating the jsdom dependency could help. It is at v16.x here, and it could be taken to v20 like in |
Not ideal but the workaround I found (which might be helpful in figuring out the issue) was this function App() {
return <div style={{ "--my-property": 1 }}>Hello</div>;
}
expect(app.style._values).toMatchObject({
"--my-property": "1", // string instead of a number
}); Reproducible repo with create react app (with the latest version of (make sure to use Node 16+) |
@testing-library/jest-dom
version: 5.1.1node
version: 12.16.2npm
(oryarn
) version: 1.22.4Relevant code or config:
Problem description:
If you were to run the test above, this would fail because
toHaveStyle
doesn't take account of CSS custom properties.Suggested solution:
I think the PR #276 caused this behavior. Indeed, the
isSubset
function has been changed and doesn't usegetPropertyValue
anymore (see this line).It seems a custom property on
CSSStyleDeclaration
can't be accessed simply using the property key (for examplecomputedStyle['--color']
), you explicitely have to usegetPropertyValue
.I forked the repo and changed back the
isSubset
function to usegetPropertyValue
and things work well. That may be a solution (I assume there's a reason why it's been changed so things may not be so simple).The text was updated successfully, but these errors were encountered: