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

Reflect on the various styling approaches #963

Open
victorlin opened this issue Jul 29, 2024 · 5 comments
Open

Reflect on the various styling approaches #963

victorlin opened this issue Jul 29, 2024 · 5 comments
Labels
proposal Proposals that warrant further discussion

Comments

@victorlin
Copy link
Member

victorlin commented Jul 29, 2024

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

@victorlin victorlin added the proposal Proposals that warrant further discussion label Jul 29, 2024
This was referenced Jul 29, 2024
@joverlee521
Copy link
Contributor

  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
Copy link
Contributor

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 😁)

@victorlin
Copy link
Member Author

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 Update to Bootstrap v4 #958 and there may be some conflicting styles now.

@victorlin victorlin mentioned this issue Jul 30, 2024
2 tasks
@genehack
Copy link
Contributor

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
Copy link
Member

  1. First consider using a global style.
    2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposals that warrant further discussion
Projects
None yet
Development

No branches or pull requests

4 participants