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

Compile CSS from Sass using Webpack #2849

Merged
merged 10 commits into from
Nov 12, 2024
Merged

Conversation

podliashanyk
Copy link
Contributor

@podliashanyk podliashanyk commented Feb 29, 2024

Implemented changes:

  • Switched to building CSS from Sass with Webpack
  • Removed libsass as dependency
  • Updated docs
  • Updated changelog
  • Clean up history

Closes #2871, and #2719

Best reviewed commit-by-commit. Furthermore this PR could be split into several smaller PRs.

Further work:

Depends on #2855, #2859

@podliashanyk podliashanyk self-assigned this Feb 29, 2024
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.42%. Comparing base (c24b90a) to head (19d7c58).
Report is 133 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2849      +/-   ##
==========================================
- Coverage   60.46%   60.42%   -0.04%     
==========================================
  Files         605      605              
  Lines       43801    43708      -93     
  Branches       48       48              
==========================================
- Hits        26483    26412      -71     
+ Misses      17306    17284      -22     
  Partials       12       12              

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

@podliashanyk podliashanyk force-pushed the build-sass-with-webpack branch from ea992a2 to e6c4790 Compare March 1, 2024 08:52
@podliashanyk podliashanyk force-pushed the build-sass-with-webpack branch 3 times, most recently from 36a822e to f7563ea Compare March 5, 2024 12:05
Copy link

github-actions bot commented Mar 5, 2024

Test results

    9 files      9 suites   8m 30s ⏱️
2 137 tests 2 137 ✅ 0 💤 0 ❌
4 013 runs  4 013 ✅ 0 💤 0 ❌

Results for commit 19d7c58.

♻️ This comment has been updated with latest results.

@podliashanyk podliashanyk force-pushed the build-sass-with-webpack branch from 23a0962 to f181d52 Compare March 5, 2024 13:23
Copy link

sonarqubecloud bot commented Mar 5, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@podliashanyk podliashanyk force-pushed the build-sass-with-webpack branch from f181d52 to 60bd9be Compare March 7, 2024 15:59
@podliashanyk podliashanyk marked this pull request as ready for review March 7, 2024 16:14
@podliashanyk podliashanyk force-pushed the build-sass-with-webpack branch from 60bd9be to f36092e Compare March 8, 2024 10:43
@hmpf
Copy link
Contributor

hmpf commented Mar 8, 2024

This is a continuation of #2859? I'd rather not review this one until that one is merged and this one updated accordingly.

@podliashanyk
Copy link
Contributor Author

This is a continuation of #2859? I'd rather not review this one until that one is merged and this one updated accordingly.

Thats right, this one is blocked by #2859. There will be a new rebase when #2859 is merged 😌

Copy link

sonarqubecloud bot commented Oct 4, 2024

Copy link

github-actions bot commented Oct 4, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 992 0 11.04s
✅ PYTHON ruff 987 0 0.1s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

@lunkwill42
Copy link
Member

I also notice there is a bunch of deprecation warnings issues by webpack when building the style sheets. Maybe we should make an issue to follow up on these as well...

@lunkwill42
Copy link
Member

I'll add that I believe setuptools_scm works just fine without setup.py these days, which means that we could potentially get fully rid of setup.py now...

@podliashanyk
Copy link
Contributor Author

podliashanyk commented Nov 7, 2024

I also notice there is a bunch of deprecation warnings issues by webpack when building the style sheets. Maybe we should make an issue to follow up on these as well...

We have issues about it: #2872 and #3167

@podliashanyk
Copy link
Contributor Author

  1. Update NAV installation docs (and release notes). Building NAV from source will now require at least Node.js and npm. The instructions need to include information on how to build the required stylesheets. Before this PR, the docs omits this step, because it is done implicitly by setup.py when using pip to install.

Will do 👍

  1. We need to look into how we could re-introduce this as an automatic part of the build process. We still use setuptools as our build tool (as specified in pyproject.toml), but we're not married to this.
  2. Ensure these changes will work with the Debian package build process (i.e. are we able to run webpack satisfactorily on Debian Bullseye during a normal package build)

I have added some issues regarding webpack's warnings and suggestions. Will you add issues for no. 2 and 3 or should I go ahead with it? :)

Also not quite sure what to think about the new frontend directory. Anyone else on the team have thoughts? @hmpf @stveit @johannaengland ?

Yes, please ❤️

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

Me like, but the bit about the sass-watcher should rather go somewhere in doc/hacking/hacking.rst, as it's not something normal users would be interested in.

doc/howto/generic-install-from-source.rst Outdated Show resolved Hide resolved
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

Yeah, I think this will do!

Now, all that remains to be seen is if we can get the Debian package build process up to snuff :)

@podliashanyk podliashanyk linked an issue Nov 8, 2024 that may be closed by this pull request
The essence of this config:
Sass files will be compiled into CSS files into the same folder (/static/css/) as it was done in the original implementation (with libsass and setuptools).

The resulting files in /static/css will have same layout and names as with the previous solution, except for /static/css/foundation folder. It is dropped completely since it was empty when built with setuptools. This is expected since /sass/foundation folder contains only Sass partials.

Named webpack configuration is used in the config. This is to drop the need of having multiple webpack config files per name/target/purpose.
* Fixes failing assets urls. 

Read more about problem here https://webpack.js.org/loaders/sass-loader/#problems-with-url. In our case asset urls must be relative to each given entry path (each generated CSS file came from a separate entry). Sass files (entries) have different path depth which introduces another problem - there can't be 1 variable that would satisfy all entries. Furthermore, some assets are defined in the same Sass file AND are used by several entries of varying depth.
squash Fix correct asset name and path


* Remove redundant imports

Otherwise webpack will fail due to assets url not being valid relative to entry path

* Fix correct import

Otherwise webpack would not build CSS file from entries


Also merges duplicate selector definitions into single definition. There were no overlaps in the merged definitions, so no unexpected CSS behaviour would occur. But merging duplicate definitions is better for code maintainability in general.
@podliashanyk podliashanyk force-pushed the build-sass-with-webpack branch 2 times, most recently from c39cd60 to e1cebe3 Compare November 12, 2024 13:49
podliashanyk and others added 7 commits November 12, 2024 14:58
Also whitelists make cmd for testing
* Updates docs about sass-watcher process in docker
* Adds docs about hacking CSS
* Adds docs on how to build CSS from Sass when building NAV from source
@podliashanyk podliashanyk force-pushed the build-sass-with-webpack branch from e1cebe3 to 19d7c58 Compare November 12, 2024 13:58
@podliashanyk podliashanyk merged commit 9245694 into master Nov 12, 2024
13 of 14 checks passed
@podliashanyk podliashanyk deleted the build-sass-with-webpack branch November 12, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend modernisation refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Webpack's watch mode or module replacement functionality We need to replace libsass
3 participants