search

nextstrain / nextstrain.org

The Nextstrain website
https://nextstrain.org
GNU Affero General Public License v3.0
89 stars 48 forks source link

Reflect on the various styling approaches #963

Open victorlin opened 3 months ago

victorlin commented 3 months ago

Currently, CSS styles are applied using various approaches listed below with examples.

It seems like most (or all?) are useful. However, when defining new styles, it's not clear what should be used. Most files I've worked on have used styled-components so that's what I've been using.

Questions for discussion

  1. Should we continue to use a mix of styling approaches? If so, when should one be used over the other?
  2. If we want to reduce the number of styling approaches, which ones do we remove and what do we replace them with?
  3. Should we consider other alternatives?

Related discussions

joverlee521 commented 3 months ago
  1. Should we continue to use a mix of styling approaches? If so, when should one be used over the other?

Global css + Boostrap are useful for keeping general styling consistent across the site without having to maintain extra "wrapper" components.

styled-component's Motivation page lists out reasons to use it. I think it's nice to encapsulate the styling to the component without any chance of it affecting other components.

inline-styling is easy to write, but hard to maintain...

  1. If we want to reduce the number of styling approaches, which ones do we remove and what do we replace them with?

I'd vote to stop using inline-styling, but no strong feelings towards the other two.

  1. Should we consider other alternatives?

Maybe if we were doing a completely overhaul of the site? Realistically though, probably not, seems like extra work for no real gain?

genehack commented 3 months ago

Adding on...

  1. Should we continue to use a mix of styling approaches? If so, when should one be used over the other?

Global css + Boostrap are useful for keeping general styling consistent across the site without having to maintain extra "wrapper" components.

+1 for striving to keep general styling consistent and generic.

styled-component's Motivation page lists out reasons to use it. I think it's nice to encapsulate the styling to the component without any chance of it affecting other components.

+1 here too

  1. If we want to reduce the number of styling approaches, which ones do we remove and what do we replace them with?

I'd vote to stop using inline-styling, but no strong feelings towards the other two.

My vote is "prefer styled components for anything that's not site-wide, avoid adding new inline, don't worry about changing existing styling until it's being touched for some other reason".

  1. Should we consider other alternatives?

Maybe if we were doing a completely overhaul of the site? Realistically though, probably not, seems like extra work for no real gain?

Hard no from me here, until and unless we're doing a from-scratch rewrite. (So, what Jover said but +2 :grin:)

victorlin commented 3 months ago

Thanks for the feedback! How about this as a rule of thumb that can be put in static-site/README.md?

  1. First consider using a global style.
    1. If the use case is covered by Bootstrap, use styled classes defined in bootstrap.css.
    2. Otherwise, add styles to globals.css.
  2. If a global style is not appropriate, use styled-components.

Potential cleanup work to keep things in line with above:

  1. Replace inline styles according to the rules above. There are many inline styles scattered across the files, so this can be done incrementally.
  2. Replace the styled component GlobalStyles with entries in globals.css.
  3. Consider moving some styles from the "global" styled-components theme file static-site/src/layouts/theme.js into globals.css or specific component files? There are also some values there that are unused and can be removed.
  4. Consider removing the injected Bootstrap v3 styles in browserCompatability.css? I missed this in #958 and there may be some conflicting styles now.
genehack commented 3 months ago

Thanks for the feedback! How about this as a rule of thumb that can be put in static-site/README.md?

1. First consider using a global style.

   1. If the use case is covered by Bootstrap, use styled classes defined in [bootstrap.css](https://github.com/nextstrain/nextstrain.org/blob/5dd3ed069c291dd3a56c9a0518f4ab6b73e2d346/static-site/src/styles/bootstrap.css).
   2. Otherwise, add styles to [globals.css](https://github.com/nextstrain/nextstrain.org/blob/5dd3ed069c291dd3a56c9a0518f4ab6b73e2d346/static-site/src/styles/globals.css).

2. If a global style is not appropriate, use styled-components.

+1 to the above.

Potential cleanup work to keep things in line with above:

1. Replace inline styles according to the rules above. There are many inline styles scattered across the files, so this can be done incrementally.

Maybe add another bullet to the guideline above, something like "if modifying something that uses inline styling, consider converting to a global style or a styled component as appropriate."

2. Replace the styled component [`GlobalStyles`](https://github.com/nextstrain/nextstrain.org/blob/5dd3ed069c291dd3a56c9a0518f4ab6b73e2d346/static-site/src/components/layout.jsx#L37) with entries in `globals.css`.

3. Consider moving some styles from the "global" styled-components theme file [static-site/src/layouts/theme.js](https://github.com/nextstrain/nextstrain.org/blob/5dd3ed069c291dd3a56c9a0518f4ab6b73e2d346/static-site/src/layouts/theme.js) into `globals.css` or specific component files? There are also some values there that are unused and can be removed.

4. Consider removing the injected Bootstrap v3 styles in [browserCompatability.css](https://github.com/nextstrain/nextstrain.org/blob/5dd3ed069c291dd3a56c9a0518f4ab6b73e2d346/static-site/src/styles/browserCompatability.css#L373)? I missed this in [Update to Bootstrap v4 #958](https://github.com/nextstrain/nextstrain.org/pull/958) and there may be some conflicting styles now.

These 3 seem like they deserve distinct issues (and personally I think the last one is much higher priority than the other two).

jameshadfield commented 3 months ago
  1. First consider using a global style.
    1. Otherwise, add styles to globals.css.

Realistically, adding styles at a global level to a site with many diverse pages is not a trivial task. If you mean adding styles scoped to a CSS class name / id then I'd suggest using CSS modules instead, and at that point the difference with Styled Components is largely ideological.

In terms of how the site feels, consistency is key. To me that means colours, margin sizes, font sizes etc that we can use throughout the site (we have a bit of this via our styled components theme), but it would also include consistent styles for UI elements like buttons. My suggestion for new work is to use as much of these site-wide styles as possible, and as we notice similarities in UI elements to lift them up into site-wide variables or reusable components. In terms of the technology to use, I'd either lean into Styled Components and use the theme styles as much as possible, or use CSS modules with global variables for consistency; given our usage of the former, it's simpler to stay on that road. Using inline-styles to override one value is just fine in my eyes.

victorlin commented 1 month ago

A few of us met in person last month and decided that we would switch from Next.js Pages Router to App Router. That means migrating away from styled-components. @genehack has started this in #1032:

This also required converting all styled-components usage in those parts over to either regular CSS/SASS, or to CSS Modules, depending on the scope of the usage. In general, I have tried to keep things scoped either very tightly (i.e., in specific CSS Modules imported into a single component) or put them in a global CSS file that is imported in the root layout.