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

introduce CSS modules #561

Merged
merged 3 commits into from
Nov 27, 2024
Merged

introduce CSS modules #561

merged 3 commits into from
Nov 27, 2024

Conversation

panentheos
Copy link
Collaborator

This converts one of the PA message CSS files to use CSS modules. The goal here was to maintain the current styling, with a few minor exceptions (a handful of hover and focus colors). While I think there are some improvements we could make to this page, I didn't want to tie those to this purely technical change. Notes:

  • Due to the explicit scoping of CSS modules, there is no possibility for accidental leakage. That means we can flatten out the selector structure and use less verbose names.
  • This leverages Bootstrap variables to modify the styling of buttons, etc. Given that we're bought into Bootstrap, this tends to encourage more consistent treatment, compared to overriding the styles explicitly.
  • Also uses builtin Bootstrap utility classes to apply common styling patterns, especially for flexbox layout and margins/padding. This reduces the need for bespoke classes in many cases.
  • Bootstrap also has the ability to generate additional utility classes, which we could use to replace some one-off classes. For example, .alertEndedText, which just sets color to a standard value, could be replaced with a generic utility if we had it.
  • Some one-off classes might be better expressed via inline styles. For example, .startEndItem, which just sets a specific flex value, could probably be an inline style.

@panentheos panentheos requested a review from a team as a code owner November 14, 2024 19:30
Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

Love this. Thanks for putting this together.

A nitpicky question I have is can we give the class names in the DOM more detailed descriptors? I imagine this is a webpack thing so not sure if it's something that can be done while we are mixing styles. I find myself using the DOM to find classes often so would be helpful, but obviously there are other ways to go about this.

--bs-btn-hover-border-color: var(--bs-btn-border-color);
}

.editPage {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we'd like to keep using kebab-case within CSS, but use the more convenient camelCase when referencing styles in JS, it looks like css-loader has an exportLocalsConvention option that can accommodate this. I'd slightly prefer this, especially given we're still using kebab-case names for mixins and variables. But it may also be convenient to have the names match up exactly for cross-referencing.

Copy link
Collaborator Author

@panentheos panentheos Nov 19, 2024

Choose a reason for hiding this comment

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

Interesting. I'd be hesitant to pay the cognitive overhead of having to translate between the two, especially since doing so would make grep-based exploration of the code more difficult. I'm curious to hear other opinions on this.

As for the generated names, yes, we can tweak that so it's more amenable to debugging.

Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

This is great. Assuming the team is all in on this, do we want to introduce a "New pages use CSS modules" policy? Then we would only have to worry about migrating existing styles.

@panentheos panentheos merged commit 753178d into main Nov 27, 2024
2 checks passed
@panentheos panentheos deleted the bhw/css-modules branch November 27, 2024 14:02
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.

3 participants