-
Notifications
You must be signed in to change notification settings - Fork 338
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
Generate theme css #1246
Generate theme css #1246
Conversation
* Add Bootstrap 4 dependency * Add scripts to compile theme files using SASS * Remove circular dependency of theme variables and Bootstrap base variables
* Rebuild the existing theme files using the new build system (93f00c9e). * Files should be functionally the same apart from possibly some Bootstrap fixes. Most of the diff is reordering and formatting (e.g. color values).
a734302
to
20c3b6b
Compare
Rebased onto current main |
@ayan4m1 I wasn't around when that decision was made, but @dessalines or @Nutomic might be able to tell you more. |
* Adapted variables to the actual color that was used before
Fixed the wrong colors in There are some very minor deviations that previously seem to be added by manually changing the compiled CSS files. But I think for future changes it's better to rely on the existing Bootstrap color transformations instead of manually defining every single color just to be exactly the same as before. @ayan4m1 Can we use |
This is a big one for us, IMO. Thanks for your work here, @fheft, let's keep it going. 👍 |
@@ -94,7 +96,7 @@ | |||
"@types/toastify-js": "^1.11.1", | |||
"@typescript-eslint/eslint-plugin": "^5.59.5", | |||
"@typescript-eslint/parser": "^5.59.5", | |||
"bootswatch": "^5.2.3", | |||
"bootstrap-v4": "npm:bootstrap@^4.6.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of bootstrap listed in the prod dependencies is 5.2.3. Should we be using v4 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a bit weird. Version 5 seems to be introduced in 15eb832 as part of regular dependency updates. But they are not compatible and there's quite a few breaking changes (I tried compiling the themes with BS5 as part of this MR and the design was completely broken...).
But we'll have to check if any of our newer code depends on BS5 features. We use two JS components (collapse and dropdown). I think I'd rather get rid of the BS5 dependency in a separate pull request to avoid making this one even more confusing.
Great work @fheft! |
This adds a formal build process for the existing theme files, which should make it easier to update the themes in the future.
Themes can be built using two new npm scripts:
themes:build
andthemes:watch
. The resulting CSS files still have to be checked into git, as compilation of multiple external SASS files is a bit hard to integrate in the current build pipeline (and I want to keep this PR as non-invasive as possible). Any ideas are welcome though 🙂There are quite a lot of non-functional changes in the compiled theme files. As far as I've seen, those are all just varying selector order or different formatting of colors (
#fff
vs#ffffff
). Let me know if there are any manual edits or hacks that I missed.[Edit] I just noticed that the–– fixed.-red
themes have a few color differences (e.g. for hover states) that are hard to reproduce using the Bootstrap variable overrides. I'll have to look into it.I also think this is a good step towards #703, even though it still uses Bootstrap 4. Switching to Bootstrap 5 is a bigger change, as there are quite a few breaking changes that lead to a lot of changes in our tsx templates...