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

OBS-383: Replace LESS with native CSS #6814

Merged
merged 3 commits into from
Dec 11, 2024
Merged

Conversation

toufali
Copy link
Contributor

@toufali toufali commented Nov 22, 2024

This PR transpiles LESS files to CSS – a preliminary step for the front-end refactor.

The following is covered:
OBS-388: transpile LESS files to CSS

OBS-389: introduce CSS custom properties – transpiling replaced LESS vars with hard-coded values. This step pulls those values back out into CSS custom properties (vars).

OBS-391: Remove the LESS compiler and all associated code – also removes all .less files and tools used to transpile.

These changes should not result in any visible difference in-browser. The app should still build without issue using django-pipeline. This was tested by following the steps in Production-style Assets, then spot-testing the site in the browser.

Note that django-pipeline is unable to process CSS @import rules. Consequently, this PR uses a single stylesheet (webapp/crashstats/crashstats/static/crashstats/css/base.css) with @imports resolved inline. This is temporary – when we introduce a capable build tool, we should delete the inlined styles and revert to @import individual stylesheets for better developer experience/organization. Additional details/breadcrumbs commented in base.css.

@toufali toufali requested a review from a team as a code owner November 22, 2024 04:21
@smarnach smarnach self-assigned this Nov 22, 2024
Copy link
Contributor

@smarnach smarnach left a comment

Choose a reason for hiding this comment

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

I'm somewhat out of my comfort zone here, so I'm not sure I understand correctly what's going on here.

It looks like this PR adds compiled versions of all the less files. And it looks like in the compiled versions all imports are inlined, and all variables are replaced with their actual values.

What I believe we eventually want is using CSS imports instead of the less imports, and CSS variables instead of the less variables, since this would keep the logical structure and maintainability of the code. Is this what you are trying to do? Or are you going some other route?

@toufali
Copy link
Contributor Author

toufali commented Nov 25, 2024

I'm somewhat out of my comfort zone here, so I'm not sure I understand correctly what's going on here.

It looks like this PR adds compiled versions of all the less files. And it looks like in the compiled versions all imports are inlined, and all variables are replaced with their actual values.

What I believe we eventually want is using CSS imports instead of the less imports,

Yes! Unfortunately CSS @import is not supported by django-pipeline. I plan to add that functionality after we install a new build tool to take django-pipeline's place. With the new tool (to come under separate PR), we should be able to fully utilize @import with tree-shaking and logical code maintainability. I left the soon-to-be obsolete LESS files in place as a temporary reference to the imports.

and CSS variables instead of the less variables,

Yes, ideally we would use CSS variables. However, the entire site style is ripe for a facelift. As I understand, the colors assigned to most/all variables are based on an obsolete "Photon" color palate. I'm happy to take a slight detour to replace all LESS vars with CSS vars, but I wonder if the app would be better served with a deeper look at style. Also, the LESS compiler doesn't offer automatic variable conversion, so the variable work would need to be manual.

In defining scope, I decided to focus more on build-tooling, architecture, dependencies, etc, leaving facelifts for future tasks. (Related, jQuery should be replaced too, but also probably out of scope for this first pass.) I intended this refactor to have a light touch on script/style, in order to confirm everything still works as expected with new tooling in place.

Happy to continue this discussion if you feel we should spend more time on CSS vars as condition for migration!

@willkg
Copy link
Contributor

willkg commented Nov 25, 2024

@smarnach I didn't realize you assigned this to yourself last week. Can you assign the review of this to me? I'm working with @toufali on this project and have a plan for validation already.

@smarnach smarnach assigned willkg and unassigned smarnach Nov 25, 2024
@smarnach
Copy link
Contributor

@willkg Sure, I didn't mean to interfere.

@toufali
Copy link
Contributor Author

toufali commented Nov 25, 2024

@smarnach I appreciated hearing your thoughts! Thanks for the look 😎

Copy link
Contributor

@willkg willkg left a comment

Choose a reason for hiding this comment

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

This PR doesn't remove LESS files, the compiler, related settings, and such. I wanted to make sure that building the image works correctly and the site serves the bundled CSS files correctly without the LESS files. Thus I pulled the changes down, rebased against current main tip, removed all the LESS files, and verified that the image builds correctly and the site works correctly.

Everything looks good. The only two changes to make are:

  1. remove the less-to-css shell script since we won't need to use it beyond this PR
  2. fix the commit comment to match our conventions:
    https://socorro.readthedocs.io/en/latest/dev.html#git-conventions

echo "converted $lessfile";
rm -f $cssfile;
lessc --global-var="root-path='$PWD/crashstats/crashstats/static/crashstats/css'" $lessfile > $cssfile;
done;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's super helpful to know how these files were converted--thank you!

However, I think it would be better to put this code in a gist or in the PR description, but not include it in the PR. This is a script we're only using for these changes and won't be using further so we don't want to maintain it.

@@ -34,5 +34,8 @@
"qs": "6.9.4",
"Select2": "github:select2/select2#3.5.4",
"tablesorter": "2.31.3"
},
"scripts": {
"convert:less": "sh less_to_css.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we're not going to have any less files after this PR lands, we don't need to add the script entrypoint.

@toufali
Copy link
Contributor Author

toufali commented Dec 5, 2024

Thanks @willkg for the review! Agreed on all.

I intended the LESS script-associated code to only exist within commit history, removing them in the second commit (along with .less files). This assumes we're not using the "squash" merge strategy 😉

@toufali toufali force-pushed the OBS-383/convert-less-to-css branch 3 times, most recently from 4405fde to be2fde9 Compare December 6, 2024 20:28
@toufali toufali requested a review from willkg December 6, 2024 21:22
@willkg
Copy link
Contributor

willkg commented Dec 9, 2024

@toufali Can you rebase this against main tip to pick up changes to obs-common and Socorro over the last couple of weeks?

Copy link
Contributor

@willkg willkg left a comment

Choose a reason for hiding this comment

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

I read through the last two commits. I ran through the site in my local dev environment. Everything looks good! 💯

@toufali toufali force-pushed the OBS-383/convert-less-to-css branch from be2fde9 to 1a00eb1 Compare December 10, 2024 18:55
@toufali toufali added this pull request to the merge queue Dec 11, 2024
Merged via the queue into main with commit 6f4d25c Dec 11, 2024
1 check passed
@toufali toufali deleted the OBS-383/convert-less-to-css branch December 11, 2024 22:05
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