fix(css): improve built-in support with css prop as function #181
Conversation
| @@ -94,13 +94,13 @@ function createGlamorous(splitProps) { | |||
| Object.freeze(this.state.theme) | |||
|
|
|||
| // create className to apply | |||
| const fullClassName = getGlamorClassName( | |||
There was a problem hiding this comment.
Just turning this into an object because there are a lot of args (and I'm going to add yet another)
| @@ -25,29 +25,21 @@ function extractGlamorStyles(className = '') { | |||
|
|
|||
| export default getGlamorClassName | |||
|
|
|||
| function getGlamorClassName(styles, props, cssOverrides, theme, context) { | |||
| function getGlamorClassName({ | |||
There was a problem hiding this comment.
to-object refactor
| props, | ||
| theme, | ||
| context, | ||
| ) | ||
| const { |
There was a problem hiding this comment.
I realized that most of this stuff was just calling into handleStyles and I could just call that once with the joined style args and that should give us the same effect. By "should," I mean, it would if the tests were passing 🙃
kentcdodds
force-pushed
the
pr/support-css-prop-as-function-better
branch
3 times, most recently
from
7cf1a47 to
b72f28e
Compare
|
Cool, I've got the build fixed, now to fix the core issue... |
| @@ -73,7 +73,7 @@ exports[`can use pre-built glamorous components with css attributes 1`] = ` | |||
| } | |||
|
|
|||
| <a | |||
| class="is added to classes css-1t62idy" | |||
| class="css-1t62idy is added to classes" | |||
There was a problem hiding this comment.
The order of the classes is not part of the supported API, so I'm ok with this.
Codecov Report
@@ Coverage Diff @@
## master #181 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 10 10
Lines 142 145 +3
Branches 35 36 +1
=====================================
+ Hits 142 145 +3
Continue to review full report at Codecov.
|
kentcdodds
force-pushed
the
pr/support-css-prop-as-function-better
branch
from
b72f28e to
069637b
Compare
**What**: This refactors things a bit and separates `css` the prop from the overrides props. **Why**: Otherwise, if the `css` prop happens to be a function, the `propsAreCssOverrides` would make the props be assigned to the function and then they'd never actually be applied. By splitting them from each other it allows us to merge both of them. **How**: Most of the work happened in `get-glamor-classname.js`. I first refactored it to simplify things a bit. Then I plugged in the `cssProp` and `cssOverrides` into their order of precidence. I also added a bit of recursion to the `handleStyles` function which allows you to pass an array of functions, this is useful for the `css` prop in particular. So you can compose functions together without a `compose` helper.
kentcdodds
force-pushed
the
pr/support-css-prop-as-function-better
branch
from
069637b to
66abb5b
Compare
kentcdodds
changed the title
|
Hey people watching the repo (anyone), could I get a few reviews? |
| const {mappedArgs, nonGlamorClassNames} = handleStyles( | ||
| styles, | ||
| [...styles, parentGlamorStyles, cssOverrides, cssProp], |
There was a problem hiding this comment.
Is this determining precedence? If so, this is the order you expect them in? Are they reduced? I would expect styles to have highest importance.
There was a problem hiding this comment.
Yes, and this preserves the original presidence. We have tests for it which you can check out. I'd link you to them, but I'm on mobile.
There was a problem hiding this comment.
No it is fine. From what I saw the tests worked as I would have expected.
|
Looks good from what I can see, but honestly not familiar enough. Not seeing anything glaring. |
|
Thanks for the review @kwelch! |
I'm working on fixing a bug I found while making this (notice that the
textAlign="center"is not applied to the first user card because thecssprop is set to a function.As part of making that an easier fix, I'm refactoring
getGlamorClassNameand I'm not sure why a bunch of the tests are breaking. I've only dug a little ways into it, but now I've got daddy duty. If anyone wants to debug it and figure out what's up, that'd be awesome. At first glance, it appears that the class names are generated properly, but somehow they're not added to glamor's stylesheet which is why they're not appearing in the snapshot???? Weird stuff.