discussion: can we drop the css and js gems?#5212
discussion: can we drop the css and js gems?#5212Bargraph6 wants to merge 17 commits intocrimethinc:mainfrom
Conversation
|
@veganstraightedge @just1602 WDYT about dropping the use of sprokets and just serving vanilla assets via propshaft? I see in the codebase there is a new UI being worked on, so maybe this is already a part of that effort? |
| gem 'erb_lint', require: false | ||
| gem 'factory_bot_rails' | ||
| gem 'fasterer', require: false | ||
| gem 'http' |
There was a problem hiding this comment.
this gem is being used in the db seeding (not sure for what). I couldn't get it running locally so just removed for the demo
There was a problem hiding this comment.
The seed script is fetching a database backup to have all the article and assets, so this is probably why it is used, tho.
There was a problem hiding this comment.
@just1602 is right
I really love the expressiveness of the http and down gems. HTTP.get() Love it.
just1602
left a comment
There was a problem hiding this comment.
Personally, I've never liked the requirement for a JS engine. I also agree with you that modern CSS allow us to do most thing SCSS allowed us to do in the past, so there's no particular reason to not switch to CSS.
It'd also allow us to remove dependencies, which is never a bad thing, IMO.
This looks really great, but I'll let @veganstraightedge make the call since he's the one leading the project.
| display: none; | ||
|
|
||
| @media screen and (min-width: $small-tablet) { | ||
| /* var(--small-tablet) */ |
There was a problem hiding this comment.
Is this because @media doesn't support the usage of var(--var-name) inside the query section, or it's because you prefer to have the explicit value?
There was a problem hiding this comment.
@just1602 you nailed it. @media queries don't support css variables. I left the commented version in the code while I was converting scss to css so that I could easily grep to get a count.
We don't use them all that often. It appears that the majority of the scss code breaks on the 800px mark (small tablet) and most of the values in viewport.(s)css are unused
There was a problem hiding this comment.
@Bargraph6 Have you tried using env() instead of var() in the media queries? It's new-ish but Can I Use says that it has pretty broad support.
https://caniuse.com/css-env-function
https://drafts.csswg.org/css-env-1/#env-function
But maybe it's too early still. TBD.
There was a problem hiding this comment.
@veganstraightedge the env() function is only for browser-defined user-agent global variables, not for user defined css variables.
the use is mostly for phones. originally developed for iphoned that had a "notch" in the screen (safe-area-inset-*), and now still mainly useful for notches and to make sure notification don't cover up important UI.
So it won't help us here
| <!-- CSS --> | ||
| <%= stylesheet_link_tag Current.theme, media: "all" unless lite_mode? %> | ||
|
|
||
| <link href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.8/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-sRIl4kxILFvY47J16cr9ZwB07vP4J8+LH7qKQnuqkuIAvNWLzeN8tE5YBujZqJLB" crossorigin="anonymous"> |
There was a problem hiding this comment.
It's not too bad, because there's the integrity attribute nowadays, but I think I'd still prefer to vendor bootstrap and be the one who serve it, but that's fine for demo’s sake.
There was a problem hiding this comment.
@just1602 agreed. otherwise we'd be leaking user's ip addresses to a third party every time they used the admin dashboard.
I did it for the demo as it was the fastest
There was a problem hiding this comment.
| gem 'erb_lint', require: false | ||
| gem 'factory_bot_rails' | ||
| gem 'fasterer', require: false | ||
| gem 'http' |
There was a problem hiding this comment.
The seed script is fetching a database backup to have all the article and assets, so this is probably why it is used, tho.
| turbo_confirm: t('admin.confirmations.delete') | ||
| } %> | ||
| class: "btn btn-danger", | ||
| method: 'delete' %> |
There was a problem hiding this comment.
What's this change about?
There was a problem hiding this comment.
@veganstraightedge this change was just for me locally and won't be included in the non-demo version of the effort.
For context, I disable javascript by default, only adding sites to my allow-list when necessary. So I was just experimenting with this fork to determine what breaks without javascript. The "delete article" button is one of the only things that break, so as an exercise for myself I wanted to see try making it work.
This change makes article deletion work without javascript by using an regular HTTP DELETE request rather than an turbo request.
There was a problem hiding this comment.
Nice! Push that up in a separate PR sometime too. I'm always happy to delete more JS!
|
@Bargraph6 @just1602 TL;DR— I'm very into it! Let's do it in small safe moves. To reduce risk. More: I've wanted to do this for a long time. But before some browser improvements, it was hard to do completely. But you're right that now is a good time to finally do this. I'm a fan of vendoring Bootstrap as well. I have a Rake task that I like to put into repos to be manually run whenever we want to bump Bootstrap. It phones home checks versions, downloads the CSS/JS compiled min files into https://gist.github.com/veganstraightedge/ecca6887f60ada828a53ee71adcbf3a9 As an exploratory PR, I think it's fine to chainsaw refactor a lot of the things. 👍🏻 But for the changes we actually ship to production, I'd like to do them in small discrete chonks. Especially because it's a very visible aspect of the site… if we get anything wrong and have issues in production. Smaller blast radius is better to recover from :) It seems to me that these are the things being done / TODO. Would you do each one in separate little PRs please? In whatever order makes the most sense to you. If some are dependent on others, it's also ok to recombine into a PR together.
🎉 |
|
@veganstraightedge sounds good to me! I have opened an initial PR here: #5216 I'll use the diff in this draft as a visual todo-list (every smaller MR that goes into main will shrink this PRs diff) |
# What are the relevant GitHub issues? related to this PR: #5212 # What does this pull request do? Most of this is just a find/replace. There is one additional change in the podcasts component where I also converted the calc to avoid mixing scss syntax with the css syntax: ```diff .latest-episode { - margin: (2 * $border-size-xlarge) auto; + margin: calc(var(--border-size-xlarge) * 2) auto; ``` # How should this be manually tested? Since this change is converting scss variables to css variables, there are only a few things that can go wrong: 1. The converted file isn't loading anymore 2. the variables are not being applied 3. specifically for the podcast change, the `calc` value is generating a different value in css vs in scss to check these things you can load the site with this change and: 1. see the new variables in the `:root` element in dev tools 2. toggle the css variables in dev tools to see that they are being applied 3. for the podcast calc value, you can verify the same value is generated manually as show in the below screenshots | before | after | |------------|---------| |  |  | |  |  | # Is there any background context you want to provide for reviewers? This is the first in what will be an extended series of commits to eventually drop the css and js pre-processor gem dependencies as discussed in #5212 This first series of PRs will remove the scss specific syntax in-place, ultimately ending in a PR the renames all of the `.scss` files to `.css` # Questions for the pull request author (delete any that don't apply): - [ ] Are any needed README/wiki/documentation updates needed for this pull request? - [ ] Does the code you updated have tests? If not, could you add some please? - [ ] Does this pull request require any new server dependencies which need to be added to the build process? If so, please explain what. # Acceptance Criteria ## These should be checked by the reviewers - [x] This pull request does not cause the database export script to become out of sync with the db schema
|
@Bargraph6 @just1602 |
ea999a2 to
95179f8
Compare
# What are the relevant GitHub issues? related to this PR: #5212 # What does this pull request do? Almost all of this is find/replace. The one additional change is in the `_colors.scss` file I removed an unused variable (`$header-color-list`) # How should this be manually tested? - Just being able to load the site without a sass error is a good sign - clicking around and seeing the colors look correct - I provided screenshots for the the two spots where I replaced a sass `lighten/darken` function with css relative color syntax: | before | after | |------------|---------| | <img width="2880" height="1568" alt="before-tools" src="https://github.com/user-attachments/assets/3237ad77-943e-406e-9eb7-6bad50069424" /> | <img width="2880" height="1559" alt="after-tools" src="https://github.com/user-attachments/assets/1ecc435e-2db0-4ede-a58c-fce22455e535" /> | | <img width="2880" height="1565" alt="before-support" src="https://github.com/user-attachments/assets/49dcfb03-0a96-48e0-8284-d6a2bdfa15c8" /> | <img width="2880" height="1567" alt="after-support" src="https://github.com/user-attachments/assets/8e7bcb07-2350-42a7-bc0b-5718e4705a88" /> | # Is there any background context you want to provide for reviewers? This is one PR in what will be an extended series of commits to eventually drop the css and js pre-processor gem dependencies as discussed in #5212 This is very similar to the border size variables PR that was merged in #5216 # Questions for the pull request author (delete any that don't apply): - [ ] Are any needed README/wiki/documentation updates needed for this pull request? - [ ] Does the code you updated have tests? If not, could you add some please? - [ ] Does this pull request require any new server dependencies which need to be added to the build process? If so, please explain what. # Acceptance Criteria ## These should be checked by the reviewers - [x] This pull request does not cause the database export script to become out of sync with the db schema
95179f8 to
126a1d7
Compare
# What are the relevant GitHub issues? related to this PR: #5212 # What does this pull request do? All of this is find/replace of SCSS comment syntax (`// comment`) with CSS comment syntax (`/* comment */`) # How should this be manually tested? - Just being able to load the site without a sass error is a good sign # Is there any background context you want to provide for reviewers? This is one PR in what will be an extended series of commits to eventually drop the css and js pre-processor gem dependencies as discussed in #5212 As referenced in the color variable PR ([#5223](#5223 (comment))), this should be the last PR in this series that touches many files at once. The next step is to convert each SCSS file to CSS files either file by file or in small groups where it make sense. # Questions for the pull request author (delete any that don't apply): - [ ] Are any needed README/wiki/documentation updates needed for this pull request? - [ ] Does the code you updated have tests? If not, could you add some please? - [ ] Does this pull request require any new server dependencies which need to be added to the build process? If so, please explain what. # Acceptance Criteria ## These should be checked by the reviewers - [x] This pull request does not cause the database export script to become out of sync with the db schema
126a1d7 to
96f7a57
Compare
What does this commit do =========================== Since the [conversion of the social icons from SCSS to CSS][0] has reached production and is working, we can move forward with renaming several `.scss` files to `.css`. **This is only a rename**. No changes are needed for these files as they already contains only CSS markup. There are other files that contain only CSS that need to be renamed. However, since they are larger files I will rename them in individual commits in case there is some SCSS syntax I missed. Changelog =========================== * 2017/articles_custom_css.css: This file is rather small, so it easy to see that it only contains CSS. I also tested this locally and it still works as a CSS file. * 2017/base/variables/_borders.scss: previously converted to css in [ba7c239][1]. Doing rename now as the social icons test worked in production. * 2017/base/variables/_colors.scss: previously converted to css in [eae7461][2]. Doing rename now as the social icons test worked in production. * 2017/components/_buttons.scss: With the previous commits to replace border and color SCSS variables with CSS variables done, this file now only contains CSS and can be renamed. * 2017/components/_colors.scss: This file is rather small, so it is easy to see that it only contains CSS. * 2017/components/_localization.scss: This file is very small (3 lines), so it is easy to see that it only contains CSS. * 2017/lib/_reset.scss: As referenced in the header comment, this file is a copy/paste of a CSS file. * 2017/lib/_utilities.scss: This file is very small (3 lines), so it is easy to see that it only contains CSS. --- related to: crimethinc#5212 related to: crimethinc#5259 related to: crimethinc#5223 related to: crimethinc#5216 [0]:crimethinc#5259 [1]:crimethinc@ba7c239 [2]:crimethinc@eae7461
What does this commit do =========================== Since the [conversion of the social icons from SCSS to CSS][0] has reached production and is working, we can move forward with renaming several `.scss` files to `.css`. **This is only a rename**. No changes are needed for these files as they already contains only CSS markup. There are other files that contain only CSS that need to be renamed. However, since they are larger files I will rename them in individual commits in case there is some SCSS syntax I missed. Changelog =========================== * 2017/articles_custom_css.css: This file is rather small, so it easy to see that it only contains CSS. I also tested this locally and it still works as a CSS file. * 2017/base/variables/_borders.scss: previously converted to css in [ba7c239][1]. Doing rename now as the social icons test worked in production. * 2017/base/variables/_colors.scss: previously converted to css in [eae7461][2]. Doing rename now as the social icons test worked in production. * 2017/components/_buttons.scss: With the previous commits to replace border and color SCSS variables with CSS variables done, this file now only contains CSS and can be renamed. * 2017/components/_colors.scss: This file is rather small, so it is easy to see that it only contains CSS. * 2017/components/_localization.scss: This file is very small (3 lines), so it is easy to see that it only contains CSS. * 2017/lib/_reset.scss: As referenced in the header comment, this file is a copy/paste of a CSS file. * 2017/lib/_utilities.scss: This file is very small (3 lines), so it is easy to see that it only contains CSS. --- related to: crimethinc#5212 related to: crimethinc#5259 related to: crimethinc#5223 related to: crimethinc#5216 [0]:crimethinc#5259 [1]:crimethinc@ba7c239 [2]:crimethinc@eae7461
What does this commit do
===========================
Since the [conversion of the social icons from SCSS to CSS][0] has
reached production and is working, we can move forward with converting
and renaming the remaining `.scss` files to `.css`.
This change converts the `_video.scss` file to CSS by :
1. unwrapping the `font-*` styles to CSS.
Changelog
===========================
* 2017/_video.css: Converted the SCSS `font {...}` syntax to CSS.
---
related to: crimethinc#5212
related to: crimethinc#5280
[0]:crimethinc#5259
What are the relevant GitHub issues? ======================= n/a What does this pull request do? ==================== * `app/views/layouts/2025/_admin_header.html.erb`: skip rendering the localization links in the admin header if the active record model doesn't define `.localizations` # How should this be manually tested? 1. **without patch** - be logged in as a publisher - set theme to 2025 - go to http://localhost:3000/categories - see error 2. **with patch applied** - be logged in as a publisher - set theme to 2025 - go to http://localhost:3000/categories - see page render, admin banner should have no localization options # Is there any background context you want to provide for reviewers? While working on pagination CSS changes related to the work to drop the sass preprocessor dependency[^1], I ran into a `500` when trying to view the categories index. In this scenario, the `@editable` is set to a `Category` model, which does not (currently) `include Translatable`. Acceptance Criteria ======================= ### These should be checked by the reviewers - [ ] This pull request does not cause the database export script to become out of sync with the db schema [^1]:#5212
* app/assets/stylesheets/2017/views/_home.scss: the home page was reaching into the pagination component to set a margin that could instead be set on the container div. * app/assets/stylesheets/2017/views/_page.scss: the _page css style's stuff like the categories index, but its css had a specificity that was reaching into pagination. * app/views/2017/articles/show.html.erb: give the surrounding div the pagination-container class to make it consistent with the rest of the site's pagination * app/views/2025/articles/show.html.erb: give the surrounding div the pagination-container class to make it consistent with the rest of the site's pagination While working on pagination CSS changes related to the work to drop the sass preprocessor dependency[^1], I ran into issue's where certain unrelated css was getting applied to the pagination component with an overriding specificity, making `.../components/_pagination.scss` no longer able to fully style the pagination component itself. I have included screenshots on the PR to try to better explain.[^2] * the 2017/2025 view changes - only the 2017 change has any real effect as the article show endpoint doesn't yet use 2025. * the `_page.scss` change - ideally the `page` css should not even need to know there is a `.pagination` class to avoid, however until `sass` is completely dropped this would be more of a headache than it is worth[^3] [^1]: crimethinc#5212 [^2]: https://github.com/crimethinc/website/pull/5296/commits [^3]: SASS and CSS handle nesting differently, even though the syntax overlaps enough for them to be somewhat compatible. The biggest difference is with what the `&` means and how it affects specificity
* app/assets/stylesheets/2017/views/_home.scss: the home page was reaching into the pagination component to set a margin that could instead be set on the container div. * app/assets/stylesheets/2017/views/_page.scss: the _page css style's stuff like the categories index, but its css had a specificity that was reaching into pagination. * app/views/2017/articles/show.html.erb: give the surrounding div the pagination-container class to make it consistent with the rest of the site's pagination * app/views/2025/articles/show.html.erb: give the surrounding div the pagination-container class to make it consistent with the rest of the site's pagination While working on pagination CSS changes related to the work to drop the sass preprocessor dependency[^1], I ran into issue's where certain unrelated css was getting applied to the pagination component with an overriding specificity, making `.../components/_pagination.scss` no longer able to fully style the pagination component itself. I have included screenshots on the PR to try to better explain.[^2] * the 2017/2025 view changes - only the 2017 change has any real effect as the article show endpoint doesn't yet use 2025. * the `_page.scss` change - ideally the `page` css should not even need to know there is a `.pagination` class to avoid, however until `sass` is completely dropped this would be more of a headache than it is worth[^3] [^1]: crimethinc#5212 [^2]: crimethinc#5296 [^3]: SASS and CSS handle nesting differently, even though the syntax overlaps enough for them to be somewhat compatible. The biggest difference is with what the `&` means and how it affects specificity
What are the relevant GitHub issues? ============================================= - related to #5212 What does this pull request do? ================================ * `app/assets/stylesheets/2017/views/_home.scss`: the home page was reaching into the pagination component to set a margin that could instead be set on the container div. * `app/assets/stylesheets/2017/views/_page.scss`: the _page css style's stuff like the categories index, but its css had a specificity that was reaching into pagination. * `app/views/2017/articles/show.html.erb`: give the surrounding div the pagination-container class to make it consistent with the rest of the site's pagination * `app/views/2025/articles/show.html.erb`: give the surrounding div the pagination-container class to make it consistent with the rest of the site's pagination Is there any background context you want to provide for reviewers? ======================================================================== While working on pagination CSS changes related to the work to drop the sass preprocessor dependency[^1], I ran into issue's where certain unrelated css was getting applied to the pagination component with an overriding specificity, making `.../components/_pagination.scss` no longer able to fully style the pagination component itself. I have included [screenshots](#5296 (comment)) on the PR to try to better explain. * the 2017/2025 view changes - only the 2017 change has any real effect as the article show endpoint doesn't yet use 2025 ([`src`][0]). * the `_page.scss` change - ideally the `page` css should not even need to know there is a `.pagination` class to avoid, however until `sass` is completely dropped this would be more of a headache than it is worth[^2] <span id="pr-screenshots">Screenshots</span> ===================================== ### `_pages.scss` If you add the following css to `_pages.scss` ```diff diff --git a/app/assets/stylesheets/2017/views/_page.scss b/app/assets/stylesheets/2017/views/_page.scss index 7926346..c395e406 100644 --- a/app/assets/stylesheets/2017/views/_page.scss +++ b/app/assets/stylesheets/2017/views/_page.scss @@ -116,8 +116,10 @@ margin-bottom: var(--border-size-large); } ul, ol { + background-color: red; + font-size: 55px; margin-bottom: 1em; } ``` you will see the following on a page such as http://localhost:3000/categories/analysis <img width="400" height="auto" alt="pages-before-2017" style="max-width: 70ch; height: auto; aspect-ratio: 16 / 9;" src="https://github.com/user-attachments/assets/8e555fc8-240a-47bb-8a0f-22f0fe0fc3dd" /> The pagination component is picking up the list styling from `_page.scss`, which we do not want. With the patched code we do not pick up the styling in the pagination component: ```diff diff --git a/app/assets/stylesheets/2017/views/_page.scss b/app/assets/stylesheets/2017/views/_page.scss index 7926346..c395e406 100644 --- a/app/assets/stylesheets/2017/views/_page.scss +++ b/app/assets/stylesheets/2017/views/_page.scss @@ -116,8 +116,10 @@ margin-bottom: var(--border-size-large); } + ul:not(.pagination), + ol:not(.pagination) { - ul, - ol { background-color: red; font-size: 55px; margin-bottom: 1em; } ``` <img width="400" height="auto" alt="pages-after-2017" src="https://github.com/user-attachments/assets/a32c7d60-baed-4ee1-a429-9888de4182e7" /> ### `_home.scss` if you make the following changes to the `_pagination` component, the effect of the `_home.scss` change is easier to see: ```diff diff --git a/app/assets/stylesheets/2017/components/_pagination.scss b/app/assets/stylesheets/2017/components/_pagination.scss index fb8cc8d..d7e99aea 100644 --- a/app/assets/stylesheets/2017/components/_pagination.scss +++ b/app/assets/stylesheets/2017/components/_pagination.scss @@ -1,4 +1,6 @@ ul.pagination { + background-color: red; + border: 2px solid black; display: inline-block; padding: 0; margin: 0; @@ -27,4 +29,6 @@ ul.pagination { .pagination-container { text-align: center; + background-color: pink; + border: 2px dashed black; } ``` with those changes, you can see the home page css is reaching into the `.pagination` class to add an internal margin: <img width="400" height="auto" alt="home-before-2017" src="https://github.com/user-attachments/assets/2642ccdb-42b3-4e92-b811-a2dfaf324c6e" /> with the patch: ```diff diff --git a/app/assets/stylesheets/2017/views/_home.scss b/app/assets/stylesheets/2017/views/_home.scss index 7be2f1e..9c72b4cc 100644 --- a/app/assets/stylesheets/2017/views/_home.scss +++ b/app/assets/stylesheets/2017/views/_home.scss @@ -35,7 +35,7 @@ } } - .pagination { + .pagination-container{ margin: 0 0 var(--border-size-xlarge) 0; } ``` the home page no longer needs to reach into `_pagination.scss`, and the resulting layout is the same: <img width="400" height="auto" alt="home-after-2017" src="https://github.com/user-attachments/assets/a11c3ee2-8381-4333-9305-54523c0ede57" /> ### `articles/show.html.erb` there are no visual changes with adding the container class to articles pagination, but I will be making use of it in a subsequent PR | theme | before | after | |------|---------|-----------| | **2017** | <img width="2880" height="1920" alt="article-before-2017" src="https://github.com/user-attachments/assets/d10da10f-c488-4eb0-92e5-5726a1c478eb" /> | <img width="2880" height="1920" alt="article-after-2017" src="https://github.com/user-attachments/assets/b2791314-42a5-4a0a-b86d-fa9623a9d175" /> | | **2025** | <img width="2880" height="1920" alt="article-before-2025" src="https://github.com/user-attachments/assets/30cbe1f9-c624-4952-b94d-82ddd2ef8683" /> | <img width="2880" height="1920" alt="article-after-2025" src="https://github.com/user-attachments/assets/117c9fcc-16f6-498e-b414-c701a1827322" /> | ## Acceptance Criteria ### These should be checked by the reviewers - [ ] This pull request does not cause the database export script to become out of sync with the db schema -------- [^1]:#5212 [^2]: SASS and CSS handle nesting differently, even though the syntax overlaps enough for them to be somewhat compatible. The biggest difference is with what the `&` means and how it affects specificity [0]:https://github.com/crimethinc/website/blob/215414e718e57f15371afeb5b6c33423ff839281/app/controllers/articles_controller.rb#L60-L77
Background
Spinning up a development environment requires getting a javascript engine installed, as the css and javascript assets require it for preprocessing.
This seems unnecessary as:
What does this pull request do?
I have been experimenting with dropping these dependencies. Things aren't perfect yet, but demo-able:
demo_compressed_265_audio_aac_crf44.mp4