Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Add styled-components, config #121

Merged
merged 2 commits into from
Oct 24, 2019

Conversation

nik-john
Copy link
Contributor

@nik-john nik-john commented Oct 21, 2019

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the master branch of Chapter.

This PR adds Styled Components for styling. This lib is fairly familiar to most folks who have been in the React community for some time. From the offical docs:

Apart from the improved experience for developers, styled-components provides:

  • Automatic critical CSS: styled-components keeps track of which components are rendered on a page and injects their styles and nothing else, fully automatically. Combined with code splitting, this means your users load the least amount of code necessary.
  • No class name bugs: styled-components generates unique class names for your styles. You never have to worry about duplication, overlap or misspellings.
  • Easier deletion of CSS: it can be hard to know whether a class name is used somewhere in your codebase. styled-components makes it obvious, as every bit of styling is tied to a specific component. If the component is unused (which tooling can detect) and gets deleted, all its styles get deleted with it.
  • Simple dynamic styling: adapting the styling of a component based on its props or a global theme is simple and intuitive without having to manually manage dozens of classes.
  • Painless maintenance: you never have to hunt across different files to find the styling affecting your component, so maintenance is a piece of cake no matter how big your codebase is.
  • Automatic vendor prefixing: write your CSS to the current standard and let styled-components handle the rest.

To add on, theming is going to be important for Chapter. We can use the built-in CSS-in-JS solution for styling but considering that folks would like to white-label their own Chapters, and for DRY and general consistency we need to have a global theme and a Styleguide and I am yet to see something that is as mature as styled-components for this. I don't see any flip sides to this as it is an overwhelmingly popular lib in the community. It's best we decide on this at the outset so we don't have get into refactor hell.

Oh and did I say Styled Components fully supports server-side rendering? ❤️

PS: I have also added a sample global theme file where we can add any global colors/shadows/text styles etc. This file can be potentially overriden if someone wants to white-label their Chapter instance.

I also see us needing some form of UI library that will help with Grid layout - suggestions welcome. styled-bootstrap seems to be a solid option

"alias": {
"client": "./client",
"server": "./server"
"server": "./server",
"styles": "./styles"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will styles/ ever be accessed from server/? if not, can we put it in client/?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forsee this to be the place where we might (in the future) add some styles that need to be rendered on the server, which is why I left this as it's own folder (I had to do this for Bootstrap on my personal project just this week). Considering that styles are oftentimes implemented on both sides of the boundary, I would recommend we leave it here. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since next.js is taking care of SSR and our current setup has us grabbing components out of client/ from pages/ where the SSR magic happens, I think it makes sense for styles to also be in client/. I haven't toyed around with any advanced config around SSR though so I'm not sure if we'd need to import anything from styles/ to the server/ directory. Would you happen to know of any instances where it would be needed to do such an import?

Copy link
Contributor Author

@nik-john nik-john Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timmyichen Sorry - I've been swamped all day. I'm just seeing this. So the scenario I was talking about is when we need to inline critical above-the-fold CSS like mentioned here. Lighthouse throws warnings when moderate sized CSS files are loaded via link as CSS imports are render-blocking. An instance of this is if we are using a light enough Grid/Flex framework, it might make sense to inline it.

Here are some instances where we do inlining: 1, 2, 3

The way we are hooking up styled-components on this PR is also by inlining the styles although it isn't as straight forward as that.

@vaibhavsingh97
Copy link
Member

I also see us needing some form of UI library that will help with Grid layout - suggestions welcome. styled-bootstrap seems to be a solid option

Can we use CSS Grid, and flexbox as a fallback ?

@vaibhavsingh97 vaibhavsingh97 self-requested a review October 21, 2019 18:44
@allella
Copy link
Contributor

allella commented Oct 21, 2019

@abidRahim mentioned using Styled Components in #11 so I'm pinging on this Styled Components topic as it might be a good time to jump in.

@ScottBrenner
Copy link
Contributor

Recommend rebasing this PR to run the proposed changes against the CI workflow!

@nik-john nik-john force-pushed the nik-john/add-styled-components branch from eee50db to c4d31c0 Compare October 22, 2019 06:56
@nik-john
Copy link
Contributor Author

@ScottBrenner Done 👍 CI workflow looks sick 🔥

@joelrozen
Copy link
Contributor

I've not used styled-bootstrap, but have implemented styled-system (https://styled-system.com/) into many projects and it is a dream to work with. I can't talk to its pros and cons, but will leave here as a suggestion

@nik-john
Copy link
Contributor Author

@here Could we merge this PR if there aren't any major comments please? I'll rebase ir In an hour

@allella
Copy link
Contributor

allella commented Oct 23, 2019

I'll defer to one of the more tech-stack aware maintainers, like @francocorreasosa, to merge this one.

Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

render() {
const { Component, pageProps } = this.props;
return (
<ThemeProvider theme={theme}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we going to provide dark theme in the future?

Copy link

@chaotic-stump chaotic-stump Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it makes sense with CSS in JS, I'd recommend with this media query prefers-color-scheme:dark {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Browser coverage is very less for prefers-color-scheme:dark {}
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's something we can look at - one of the advantages of using a global theme is that it's one source of truth for the entire application and whitelabelling and re-theming are a walk in the park

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, in that case, I'd favor something with more adoption then; like css vars you could change with a toggle or something. I'm sure ThemeProvider is fine too.

]
]
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}

@allella allella mentioned this pull request Oct 23, 2019
18 tasks
@nik-john
Copy link
Contributor Author

Merging this in now, will raise another PR for the UI lib

@nik-john nik-john merged commit 1296360 into freeCodeCamp:master Oct 24, 2019
This was referenced Oct 28, 2019
@nik-john
Copy link
Contributor Author

@all-contributors please add @nik-john for doc tool

@allcontributors
Copy link
Contributor

@nik-john

I've put up a pull request to add @nik-john! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants