feat(getValueDoppelganger): Ignore css property values with var()
#62
Conversation
Fixes # Since css variables can be more or less anything, added an extra utility function to match for the presence of `var()` in the css property value, since there's really no way to know how to actually compute the RTL doppelganger. I was initially worried that `translateX(var())` might be an edge case that needs to be handled, but after testing, the result is currently the same since the regexp will never match with the `var()` being used inside the `translateX` call. Since the original issue is only with `boxShadow` or `textShadow` I'm open to any suggestions that move the `containsCssVar` check only for that property value converter.
| @@ -431,6 +431,12 @@ const unchanged = [ | |||
| [{opacity: () => {}}], | |||
| [{objectPosition: 'center bottom'}], | |||
| [{objectPosition: '5px 10px'}], // There's no RTL-flipped equivalent for the 5px. :-( | |||
| [{boxShadow: 'var(--shadow16)'}], | |||
There was a problem hiding this comment.
with the exception of boxShadow the below tests were all passing before and after the changes in this PR
|
Thanks for this! I hadn't thought about css variables. I wonder if it would ever make sense for someone to have two different variables assigned and hope that they would get swapped for them: Do you think there'd be a way to support this? |
In the scope of a library that does transforms, that seems to me like some shaky logic. Wouldn't it just be string replacing any variable we see with There might also be other libraries that follow Perhaps I'm misunderstanding you, an example might help in that case |
|
Nah, you're right. That would not be advisable to rely on. Let's go with this as a bug fix 👍 |
Codecov Report
@@ Coverage Diff @@
## main #62 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 220 225 +5
Branches 41 43 +2
=========================================
+ Hits 220 225 +5
Continue to review full report at Codecov.
|
|
@kentcdodds thx for merging, just wondering when this would be released? Also did I add myself to contributors correctly? |
|
@all-contributors please add @ling1726 for code and tests |
|
I've put up a pull request to add @ling1726! 🎉 |
|
Sorry about that. I'm not sure why a release didn't go out and I forgot to fix the contributor thing. I use a bot so you don't have to add yourself manually :) |
|
Thx! Couldn't ask for more for a first time oss contribution |
There was an issue with a patch release, so this manual-releases.md change is to release a new patch version. Reference: #62
|
We're having trouble with the release: https://github.com/kentcdodds/rtl-css-js/runs/4197260969?check_suite_focus=true#step:7:137 I'm not sure why this is happening 😬 I'll figure out how to get it fixed though. |
There was an issue with a patch release, so this manual-releases.md change is to release a new patch version. Reference: #62
|
Fixed and released! |
What:
Fixes #61 which illustrates a concrete example of the problem
Since the original issue is only with
boxShadowortextShadowI'mopen to any suggestions that move the
containsCssVarcheck only forthat property value converter.
How:
Created an extra
containsCssVarregex match and used it with the existing checksfor boolean and undefined property values. Extracted the check to a separate utility
to avoid too much complexity in the function.
I was initially worried about some edge cases:
translateX(var())padding: '2px var(--foo)';but after testing, it seems that the results are the same before and after the changes in this PR.
Checklist: