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

Simplify CSS (remove main.module.css) #675

Merged
merged 4 commits into from
May 14, 2021
Merged

Simplify CSS (remove main.module.css) #675

merged 4 commits into from
May 14, 2021

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented May 13, 2021

Fixes #80 (see issue for details)

@zbynek zbynek requested a review from a team as a code owner May 13, 2021 11:43
@halkeye
Copy link
Member

halkeye commented May 14, 2021

hrmmmmm, at some point when I created #80, I was really annoyed at the !importants, and eventually I came around on the css modules.

I'm absolutely cool with this PR once its updated with latest master, but we should also decide if we want to do modules or not. There's 3 other files that I could pull out in a followup PR.

https://github.com/jenkins-infra/plugin-site/blob/master/src/components/Checksum.module.css
https://github.com/jenkins-infra/plugin-site/blob/master/src/components/InstallViaCLI.module.css
https://github.com/jenkins-infra/plugin-site/blob/master/src/components/SearchByAlgolia.module.css

The nice thing about css modules is that they are contained and a module doesn't care where its used, it just has css applied. But at the same time, it makes it harder to see whats going on.

So feelings?

@halkeye
Copy link
Member

halkeye commented May 14, 2021

a couple minor nitpicky stuff, not blocking the {''} to "" stuff, i just feel its cleaner

zbynek and others added 3 commits May 14, 2021 05:19
Co-authored-by: Gavin Mogan <github@gavinmogan.com>
Co-authored-by: Gavin Mogan <github@gavinmogan.com>
@zbynek zbynek merged commit 079d2c9 into master May 14, 2021
@zbynek zbynek deleted the remove-css-module branch May 14, 2021 04:25
@zbynek
Copy link
Contributor Author

zbynek commented May 14, 2021

I think modules are fine if they have clearly defined responsibilities, i.e. classes defined in the module are not present in base.css and the module dos not contain classes for unrelated components. Maybe we could do modules for new components and keep the non-module CSS files for old stuff. Also we're loading bootstrap by default, so we can leverage some of the classes it defines like mt-3 and avoid creating own classes just for setting margin.

@halkeye halkeye added the bug label Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Completely remove the main.module.css
3 participants