#212 - remove theming #225
Conversation
- first pass
| background-color: #ffd43b; | ||
| color: #8c81d8; | ||
| height: 64px; | ||
| font-size: 32px; | ||
| } | ||
|
|
||
| <span | ||
| className="glamor-0 glamor-1" | ||
| className="glamor-0 glamor-1 glamor-2 glamor-3" |
There was a problem hiding this comment.
@mitchellhamilton I'm not sure how to interpret these snapshot diffs. Do they look right to you?
There was a problem hiding this comment.
They don't, there should only be two classes on each component.
There was a problem hiding this comment.
This is the actual output on a web page. Maybe this will have a clue.
There was a problem hiding this comment.
Notice the difference in the length of the hash.
There was a problem hiding this comment.
This is due to wrapping the components with withTheme I believe. I tested this out on the browser and it works as expected.
There was a problem hiding this comment.
Yes, that's correct because they're wrapped in withTheme so more components are rendered and the class is added for using components as selectors.
Now that I think about it, we might want to always add the class for using components as selectors because in this example OneComponent will have color: yellow; when rendered inside of SomeComponent but AnotherComponent will not.
const OneComponent = styled.div`
display: flex;
`
const AnotherComponent = styled(OneComponent)`
background-color: hotpink;
`
const SomeComponent = styled.div`
${OneComponent} {
color: yellow;
}
`There was a problem hiding this comment.
Do you think I should hold up merging this for that fix or is it something we can separate?
There was a problem hiding this comment.
I already pushed a fix to this branch 😀
Codecov Report
@@ Coverage Diff @@
## master #225 +/- ##
==========================================
- Coverage 90.39% 90.27% -0.12%
==========================================
Files 22 20 -2
Lines 979 967 -12
Branches 265 262 -3
==========================================
- Hits 885 873 -12
Misses 76 76
Partials 18 18
Continue to review full report at Codecov.
|
Putting this up early to get more eyes on it.
Checklist: