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

Generate theme css #1246

Merged
merged 8 commits into from
Jun 17, 2023
Merged

Generate theme css #1246

merged 8 commits into from
Jun 17, 2023

Conversation

fheft
Copy link
Contributor

@fheft fheft commented Jun 13, 2023

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 and themes: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 -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. –– fixed.

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...

@fheft fheft changed the title Generate theme css Draft: Generate theme css Jun 13, 2023
fheft added 2 commits June 14, 2023 01:14
* 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).
@fheft fheft force-pushed the generate-theme-css branch from a734302 to 20c3b6b Compare June 13, 2023 23:14
@fheft
Copy link
Contributor Author

fheft commented Jun 13, 2023

Rebased onto current main

@ayan4m1
Copy link
Contributor

ayan4m1 commented Jun 15, 2023

@fheft I'm new to the codebase, so apologies for the question, but is there any reason we couldn't add sass/sass-loader as a dependency and use something like this to import bootswatch/bootstrap SCSS for each theme that is getting ported over?

@SleeplessOne1917
Copy link
Member

@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
@fheft fheft changed the title Draft: Generate theme css Generate theme css Jun 16, 2023
@fheft
Copy link
Contributor Author

fheft commented Jun 16, 2023

Fixed the wrong colors in litely-red. Now all colors should be the same as before this branch or at least match visually.

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 sass-loader to build separate CSS files for each theme during build time? I think it's beneficial to keep having separate theme CSS files instead of bundling them all together into one big styles.css, e.g. only loading what's actually used (smaller network footprint).

@alectrocute
Copy link
Contributor

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

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?

Copy link
Contributor Author

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.

@SleeplessOne1917 SleeplessOne1917 merged commit 7407858 into LemmyNet:main Jun 17, 2023
@Zetaphor
Copy link
Contributor

Great work @fheft!

@fheft fheft mentioned this pull request Jun 17, 2023
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.

5 participants