Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: styled-components #434

Merged
merged 8 commits into from
May 24, 2018
Merged

RFC: styled-components #434

merged 8 commits into from
May 24, 2018

Conversation

HipsterBrown
Copy link
Contributor

@HipsterBrown HipsterBrown commented May 23, 2018

TL;DR:

There is a lot of repeated and inconsistent styles spread around the application. Using an extensible library like styled-components and its ecosystem will make building UIs efficient and consistent.

Motivation:

While working on the layout and design of the new homepage (as well as the search page previously), I had some trouble learning the best practice for using a responsive grid with repeating styles and modifying existing component styles without exposing global selectors.

styled-jsx is great at scoping selectors to the component, but it has not been useful for sharing common styles and creating extensible UI components.

The react-bootstrap grid components allows for responsive width sizing but does not have a nice API for working with flexbox or spacing utilities (Bootstrap v4 has this but not coming anytime soon to react-bootstrap).

Solution:

While searching for a solution to these issues, I looked to one of my favorite CSS libraries, basscss for inspiration and discovered a simple yet powerful grid library, built by the author of basscss, called grid-styled that uses styled-components as a peer dependency.

styled-components works with Next.js, including server-side rendering, and allows for styling third-party components. I believe adopting this library and pattern will cleanup a lot of our css-in-js code with no apparent impacts in performance.

We can also use it alongside styled-jsx while we update existing components.

Alternatives:

I tried to recreate the API provided by grid-styled using styled-jsx however dynamically creating media queries and making the components extensible required more custom tooling than it was worth.

Proof of Concept:

Included in this PR is an example of using styled-components and grid-styled to refactor the SearchPage. The only custom css required was written to customize the FormControl from react-bootstrap.

Adoption / Transition Strategy:

Because we can use styled-components and styled-jsx together in the app, we should be able to completely adopt styled-components over time. As we work on files that use styled-jsx, one refactor commit should be made to convert the file to use styled-components. This will probably start with replacing the global styles in the _document.js page and the <Header> component with an injectGlobal declaration and a theme with our common colors, font sizes, spacing values, etc.

I have not been able to find an automated solution for converting styled-jsx blocks into styled-components.

Copy link
Contributor

@clarete clarete left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@znarf
Copy link
Member

znarf commented May 24, 2018

So the end resolution is to ultimately transition all styled-jsx to styled-components?

If yes, what do you think would be the appropriate transition strategy?

  • a) progressively when we have time?
  • b) at some point, we take the time to migrate everything?

Would some kind of automation make sense for the transition?

I would move the decision about bootstrap vs rebass to another RFC.

Copy link
Member

@znarf znarf left a comment

Choose a reason for hiding this comment

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

I'm favorable to it.

  • I would like to have an idea of the transition strategy
  • I would propose to separate RFCs for styled-components and basscss
  • Would be great to wrap up the RFC in a txt file, eg: rfc/002-styled-components. For future developers to easily browse that kind of documents in one place.

@HipsterBrown
Copy link
Contributor Author

@znarf I've added the RFC doc.

I would propose to separate RFCs for styled-components and basscss

This RFC is only about including styled-components. I referenced basscss as inspiration for a solution, sorry for the confusion.

I would move the decision about bootstrap vs rebass to another RFC.

I agree. I removed the reference to potentially moving to rebass and will compose another RFC when appropriate.

Let me know if you have any other questions.

@znarf
Copy link
Member

znarf commented May 24, 2018

I think it's great to start with the styled-components and styled-jsx in parallels, smooth transition. No need to hurry, but I would not like to have that for too long.

We should put the removal of styled-jsx in our roadmap. Maybe creating a ticket to track that should be enough for now.

Our styled-jsx setup is currently configured with postcss, do we want to keep that? It's barely used right now.

@HipsterBrown
Copy link
Contributor Author

@znarf

We should put the removal of styled-jsx in our roadmap. Maybe creating a ticket to track that should be enough for now.

I will do that and add it to the description of this PR. 👍

Our styled-jsx setup is currently configured with postcss, do we want to keep that? It's barely used right now.

It appears we are only using postcss to enable nesting of rules in styled-jsx, which styled-components allows automatically, so we can remove that as well.

@znarf
Copy link
Member

znarf commented May 24, 2018

@HipsterBrown A few days ago, I was not far from proposing to drop postcss, we actually had only 2 nested rules in the codebase. But I ultimately thought it could be useful and we better have a discussion about it first. Definitely up for it.

@HipsterBrown HipsterBrown merged commit 1faaeb2 into master May 24, 2018
@HipsterBrown HipsterBrown deleted the rfc-styled-components branch May 24, 2018 20:33
@xdamman
Copy link
Contributor

xdamman commented May 25, 2018

I really like this rfc process. Great write up @HipsterBrown and good suggestions @znarf!
(and thanks @clarete for always being up and supportive of everything! 👍👍👍)

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

Successfully merging this pull request may close these issues.

None yet

4 participants