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

Themeing Enhancement: Extract and consolidate colors, create css variables #1226

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

schlawiner
Copy link
Contributor

In order to ease retheming in a custom stylesheet, I extracted all the colors used in the Skosmos styles.css to CSS variables, so they can be replaced in one location (by overwriting the variable value in your custom stylesheet).

Some gray tones were so close to each other and were used in only one place that I consolidated a few into one color variable.

@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Great improvement! I used some variables in the PR to update bootstrap too, good idea 👍

@osma
Copy link
Member

osma commented Nov 10, 2021

@kinow Would merging this now interfer with the development of the Bootstrap 5 update (#1182)? This seems like a very useful improvement, but I'm wondering whether to do it now or to wait for the similar changes in that PR.

@kinow
Copy link
Collaborator

kinow commented Nov 10, 2021

@kinow Would merging this now interfer with the development of the Bootstrap 5 update (#1182)? This seems like a very useful improvement, but I'm wondering whether to do it now or to wait for the similar changes in that PR.

@osma , I don't mind rebasing and fixing conflicts after this change (thanks for checking though). It really seems like a great improvement.

@osma
Copy link
Member

osma commented Nov 11, 2021

Thanks @schlawiner for the PR and @kinow for confirming that this doesn't cause too much trouble with the Bootstrap 5 update.

I've tested this and it seems to work well. I have some doubts about the variable naming (white-color, light-color and dark-color aren't particularly semantic...) but I don't think it makes sense to get stuck on that - we can rename those later if necessary. This is obviously a huge win for theming in any case. So I'm merging this as-is. 🎉

@osma osma self-assigned this Nov 11, 2021
@osma osma added this to the 2.13 milestone Nov 11, 2021
@osma osma merged commit bdf7bda into NatLibFi:master Nov 11, 2021
@kinow kinow mentioned this pull request Nov 16, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants