Skip to content

discussion: can we drop the css and js gems?#5212

Draft
Bargraph6 wants to merge 17 commits intocrimethinc:mainfrom
Bargraph6:drop-css-and-js-gems
Draft

discussion: can we drop the css and js gems?#5212
Bargraph6 wants to merge 17 commits intocrimethinc:mainfrom
Bargraph6:drop-css-and-js-gems

Conversation

@Bargraph6
Copy link
Copy Markdown
Contributor

@Bargraph6 Bargraph6 commented Mar 23, 2026

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:

  • for the css: vanilla css is pretty full featured nowadays
  • none of our JS really needed to be preprocessed. It can just be served from propshaft
  • the bootstrap stuff can either be loaded in from a cdn (what I do in the demo for simplicity) or vendored in

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
some notes one the demo:
I block browser fonts, so that is why they look the way they do. Also, had to compress the video quite a bit to get under github limits

@Bargraph6
Copy link
Copy Markdown
Contributor Author

@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?

Comment thread Gemfile
gem 'erb_lint', require: false
gem 'factory_bot_rails'
gem 'fasterer', require: false
gem 'http'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The seed script is fetching a database backup to have all the article and assets, so this is probably why it is used, tho.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@just1602 is right

remote_tool_json = HTTP.get(remote_tool_url).to_s

I really love the expressiveness of the http and down gems. HTTP.get() Love it.

Copy link
Copy Markdown
Collaborator

@just1602 just1602 left a comment

Choose a reason for hiding this comment

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

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) */
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@Bargraph6 Bargraph6 Mar 24, 2026

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Contributor

@veganstraightedge veganstraightedge Mar 24, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread Gemfile
gem 'erb_lint', require: false
gem 'factory_bot_rails'
gem 'fasterer', require: false
gem 'http'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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' %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's this change about?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Push that up in a separate PR sometime too. I'm always happy to delete more JS!

@veganstraightedge
Copy link
Copy Markdown
Contributor

@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 vendor/*.

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.

  • Vendor Bootstrap JS/CSS files
  • Remove bootstrap gem
  • Remove autoprefixer-rails gem
  • Remove uglifier gem
  • Convert .scss to modern vanilla CSS (now with nesting and variables! ✨) (w/ 2 spaces indentation pls)
  • Remove sassc-rails gem
  • Clean up config/initializers/assets.rb
  • Add propshaft gem
  • Move from app/javascript to app/assets/javascripts
  • Remove manifest.js

🎉

@Bargraph6
Copy link
Copy Markdown
Contributor Author

@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)

veganstraightedge pushed a commit that referenced this pull request Mar 26, 2026
# 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 |
|------------|---------|
|
![before1-compressed](https://github.com/user-attachments/assets/08d2a0e5-85dc-4bf8-b711-65f98c66ceeb)
|
![after1-compressed](https://github.com/user-attachments/assets/5fad4358-1690-4dd2-a870-fadccc3d537f)
|
|
![before2-compressed](https://github.com/user-attachments/assets/b95b75e9-9e75-4b8e-bc72-ab99864e61f6)
|
![after2-compressed](https://github.com/user-attachments/assets/e66359e4-6b73-4d6d-bce2-b156d4be7271)
|

# 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
@veganstraightedge
Copy link
Copy Markdown
Contributor

@Bargraph6 @just1602
If it's helpful, I made this gem to help with vendor Bootstrap JS and CSS:

https://github.com/xoengineering/bootstrap-vendor

@Bargraph6 Bargraph6 force-pushed the drop-css-and-js-gems branch from ea999a2 to 95179f8 Compare March 26, 2026 10:06
veganstraightedge pushed a commit that referenced this pull request Apr 2, 2026
# 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
@Bargraph6 Bargraph6 force-pushed the drop-css-and-js-gems branch from 95179f8 to 126a1d7 Compare April 14, 2026 08:58
just1602 pushed a commit that referenced this pull request Apr 14, 2026
# 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
@Bargraph6 Bargraph6 force-pushed the drop-css-and-js-gems branch from 126a1d7 to 96f7a57 Compare April 14, 2026 13:24
@Bargraph6 Bargraph6 mentioned this pull request Apr 14, 2026
4 tasks
Bargraph6 added a commit to Bargraph6/website that referenced this pull request Apr 20, 2026
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
just1602 pushed a commit to Bargraph6/website that referenced this pull request Apr 20, 2026
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
Bargraph6 added a commit to Bargraph6/website that referenced this pull request Apr 23, 2026
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
just1602 pushed a commit that referenced this pull request Apr 24, 2026
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
Bargraph6 added a commit to Bargraph6/website that referenced this pull request Apr 24, 2026
* 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
Bargraph6 added a commit to Bargraph6/website that referenced this pull request Apr 24, 2026
* 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
just1602 pushed a commit that referenced this pull request Apr 24, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants