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

Precompile CSS files into Gem #2886

Merged
merged 16 commits into from
Jun 26, 2024
Merged

Precompile CSS files into Gem #2886

merged 16 commits into from
Jun 26, 2024

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented May 23, 2024

What is this pull request for?

Check in compiled CSS in the gem and do not compile the Sass assets within the context of the host app anymore.

Notable changes

Removes the sassc-rails dependency 🥳

This also removes the ability to add additional .scss files into the admin css bundle, as well as to overwrite Sass variables.

But this is probably a rare use case anyway and the pros outweigh the cons here.

TODO

  • Add watching of CSS changes to bin/start command.
  • Add local build instructions to README
  • Figure out how to overwrite our custom properties (maybe add them to the body scope)?

Checklist

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.96%. Comparing base (cb5841d) to head (e0102ae).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2886   +/-   ##
=======================================
  Coverage   95.96%   95.96%           
=======================================
  Files         232      232           
  Lines        6272     6272           
=======================================
  Hits         6019     6019           
  Misses        253      253           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@afdev82
Copy link
Contributor

afdev82 commented Jun 25, 2024

Is there something that I could do to help this PR to be merged?
I am currently switching my application too to dartsass-rails, but because AlchemyCMS still requires sassc-rails, I can't.
The asset pipeline still tries to compile my already compiled builds (and it throws errors).

@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 25, 2024

Is there something that I could do to help this PR to be merged? I am currently switching my application too to dartsass-rails, but because AlchemyCMS still requires sassc-rails, I can't. The asset pipeline still tries to compile my already compiled builds (and it throws errors).

@afdev82 We still need to decide on how we build and ship the compiled assets w/o using the host app for compilation. If you have any idea I would love hear from it

tvdeyen added 13 commits June 25, 2024 17:43
Sass maintains a global namespace. If we want to be able to have a
frontend "alchemy/elements" import we would introduce conflicts.
We dont want to use Sass indented syntax in our project
This file should never be compiled on its own
Normal Sass is not able to import with wildcards, while sass-rails
added this functionality.
This is the modern replacement for sassc-rails that uses the Dart
implementation instead of libsass.
These files are already processed and do not contain any Sass specific code
And serve them via Sprockets
We already have prebuild files that we can just serve via
Sprockets.
We do not compile them via Sprockets anymore.
The print styles are loaded in its dedicated file and must
not override the admin styles.
We cannot expect a preprocessor in the host app
- Error: Silent comments aren't allowed in plain CSS.
- Deprecation warning: Values without unit not allowed
- Fix calc usage in tables padding
@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 25, 2024

@afdev82 so, I took some time today to rebase this with latest main. Local testing looks promising. Do you mind to test this branch in your app and provide feedback? Thanks

@tvdeyen tvdeyen force-pushed the dart-sass branch 5 times, most recently from 9c01b43 to da9ba71 Compare June 25, 2024 20:21
In order to solve

    undefined method `parse' for Psych::Parser

errors while installing sass-embedded.
@tvdeyen tvdeyen changed the title Dart sass Prebuild CSS files Jun 25, 2024
@tvdeyen tvdeyen changed the title Prebuild CSS files Precompile CSS files into Gem Jun 25, 2024
@tvdeyen tvdeyen marked this pull request as ready for review June 25, 2024 20:44
@tvdeyen tvdeyen requested a review from a team as a code owner June 25, 2024 20:44
@tvdeyen tvdeyen added this to the 7.3 milestone Jun 25, 2024
Rakefile Outdated Show resolved Hide resolved
app/assets/builds/alchemy/welcome.css Show resolved Hide resolved
@afdev82
Copy link
Contributor

afdev82 commented Jun 26, 2024

Hi, thank you.
I will try this branch today.
My first idea was to precompile the admin assets in one css and provide it with the gem, exactly what you are doing now I see.

This also removes the ability to add additional .scss files into the admin css bundle, as well as to overwrite Sass variables.
But this is probably a rare use case anyway and the pros outweigh the cons here.

Yes, that's exactly what I am doing in my app 😆
I have the all.css manifest file in the vendor folder, that includes alchemy/admin and a custom file.
Now I could simply append a stylesheet_link_tag with the custom file, it's quite the same.

@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 26, 2024

Hi, thank you. I will try this branch today. My first idea was to precompile the admin assets in one css and provide it with the gem, exactly what you are doing now I see.

This also removes the ability to add additional .scss files into the admin css bundle, as well as to overwrite Sass variables.
But this is probably a rare use case anyway and the pros outweigh the cons here.

Yes, that's exactly what I am doing in my app 😆 I have the all.css manifest file in the vendor folder, that includes alchemy/admin and a custom file. Now I could simply append a stylesheet_link_tag with the custom file, it's quite the same.

Kind of, but you would need to inject that stylesheet_link_tag into the admin.html.erb file with either a content_for or something like Deface. Maybe we can provide something more robust, but this can be tackled later, I guess.

Thanks for checking out.

@afdev82
Copy link
Contributor

afdev82 commented Jun 26, 2024

Yes, it's not a big deal I think. If I want to overwrite a style in all views, I could also overwrite the admin layout to include the file there. With updates it's not ideal, but I don't think there will be much going on in the admin layout file.
At the moment I am playing around in the admin interface and it seems all good 👍

@tvdeyen tvdeyen merged commit b39d302 into main Jun 26, 2024
36 checks passed
@tvdeyen tvdeyen deleted the dart-sass branch June 26, 2024 07:40
@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 26, 2024

@afdev82 just merged into main. Make sure to update your Gemfile

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