Replace Radium with Emotion 👩🎤 #394
Conversation
|
👏 |
jstcki
changed the title
There was a problem hiding this comment.
Looks good, at least on the code level, I haven't looked at how it runs.
When I started the server, this circular dependency was brought to my attention:
(!) Circular dependency: src/components/Menu/ListItem.js -> src/components/Menu/NestedList.js -> src/components/Menu/ListItem.js
(!) Circular dependency: src/components/Menu/Menu.js -> src/components/Menu/ListItem.js -> src/components/Menu/NestedList.js -> src/components/Menu/Menu.js
|
|
||
| const context = typeof global !== "undefined" ? global : {}; | ||
|
|
||
| if (context.__CATALOG_EMOTION_INSTANCE__ === undefined) { |
There was a problem hiding this comment.
I was already wondering how to resolve a conflict of multiple Emotion instances – and then I saw this. Perfect.
| [`${selector} b, strong`]: { | ||
| fontWeight: 700 | ||
| }, | ||
| [`${selector} a, a:visited`]: { |
There was a problem hiding this comment.
In the new Content/Markdown.js we no longer handle a:visited from what I can tell, but we should.
There was a problem hiding this comment.
It doesn't seem necessary – at least in Chrome and FF 🤔
Also, these selectors didn't even work properly! They should've been ${selector} a, ${selector} a:visited etc.
| ...inlineElements(theme, `${selector} >`) | ||
| }); | ||
|
|
||
| export const unorderedList = (theme, selector = "ul", level = 0, depth = 0) => { |
There was a problem hiding this comment.
I can't see how nested lists are handled in the new Content/Markdown.js – does it still work? I think it should …
There was a problem hiding this comment.
Each component "owns" its styling now. We don't need to select nested lists from the top like .cg-Page > ul > li > ul anymore.
List items just remove the top and bottom margin of their first/last child respectively.
| @@ -52,8 +52,8 @@ class Loader extends React.Component { | |||
| render() { | |||
| const loader = this.state.visible ? styles.spinner : styles.hidden; | |||
|
|
|||
| return <div style={loader} className="cg-Page-Loader" />; | |||
There was a problem hiding this comment.
Did .cg-Page-Loader not have styles before? I wanted to make sure we did not forget to remove these obsolete styles, but I could not find them ¯_(ツ)_/¯
There was a problem hiding this comment.
As far as I can tell, all the .cg-* classes were either leftovers from before Radium or necessary for creating deep selectors like .cg-Page > p. There's no reference to cg- anywhere in the code anymore.
|
@grossbart Thanks for the review! The circular dependency was already present before this PR. It hasn't been a problem so far, but we should remove it. But let's do that in another PR 😉 To check functionality you can compare https://deploy-preview-394--catalog-docs.netlify.com/ and https://docs.catalog.style – I've tried to reproduce the original styling accurately and I'm not aware of any discrepancies. A second pair of 👀would definitely be useful! |
Replace Radium with Emotion to solve long-standing issues like not being able to statically render pages and rendering issues with media queries.
This PR is pretty much a direct translation of the Radium styles to Emotion, i.e. in most of the places I just replaced
style={x}withclassName={css(x)}. In future updates we can migrate to template string styles or styled components if desired.For the elements on Markdown pages I got rid of the complicated nested page styles generated in
styles/typography.jsin favor of styled elements (I didn't use react-emotion directly, as the use case here is slightly different and simpler).