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

Fix CSS overrides in applications #12646

Merged
merged 14 commits into from
Apr 16, 2024
Merged

Fix CSS overrides in applications #12646

merged 14 commits into from
Apr 16, 2024

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Mar 23, 2024

🎩 What? Why?

When we have switched away from webpacker to Shakapacker back into #10389, we accidentally introduced a bug regarding the CSS overrides. This may not be visible at the first hand, but when you try to create overrides for component css ( meetings, proposals), you are forced to use !important.

As per current implementation, the decidim_application.scss is being imported as part of decidim_core.scss. However, in the Decidim + Shakapacker world, the component specific css is being required as a standalone file, which means that the css overrides is being loaded before the code that is supposed to override.

This PR fixes this by adding 3 new entrypoints: decidim_overrides, decidim_admin_overrides and decidim_system_overrides, which are loaded after the other css has been loaded.

Basically, in the frontend context:

<link rel="stylesheet" media="all" href="/decidim-packs/css/decidim_core.css" />
<link rel="stylesheet" media="all" href="/decidim-packs/css/decidim_meetings.css" />

Becomes:

<link rel="stylesheet" media="all" href="/decidim-packs/css/decidim_core.css" />
<link rel="stylesheet" media="all" href="/decidim-packs/css/decidim_meetings.css" />
<link rel="stylesheet" media="all" href="/decidim-packs/css/decidim_overrides.css" />

This will allow any user to properly customize the front-end, admin or system areas of the platform.

📌 Related Issues

Link your PR to an issue

Testing

  1. Start the application
  2. Visit the project homepage
  3. Inspect the upcoming meetings widget classes and identify meeting-list__block-list class
  4. Inspect the applied css, and see the display: property ( it should be display: flex)
  5. Edit app/packs/stylesheets/decidim/decidim_application.scss
  6. Add an override like .meeting-list__block-list { display: block; }
  7. Refresh and repeat steps 3 & 4 , observe the active css display rule is still flex
  8. Our override is like 3rd in the applied rules, and is being overwritten
  9. Apply patch, and restart the application
  10. Repeat steps 3 & 4. Identify the overridden class, observe the display property.
  11. It should be display: block

♥️ Thank you!

@alecslupu alecslupu added the type: fix PRs that implement a fix for a bug label Mar 23, 2024
github-actions[bot]
github-actions bot previously approved these changes Mar 23, 2024
github-actions[bot]
github-actions bot previously approved these changes Mar 23, 2024
github-actions[bot]
github-actions bot previously approved these changes Mar 23, 2024
@alecslupu alecslupu marked this pull request as ready for review March 23, 2024 17:38
@andreslucena
Copy link
Member

@alecslupu I think there are actually two things here:

  1. A bug that we can't override without using !important. I think we should fix that for v0.28
  2. A new feature, where we're adding specific files for overriding in system, admin and core in a separate way. This could not be backported as it's a feature.

I think it'd be cleaner to work focus first in the bug fix (case 1), backport that and then work in the feature (case 2), do you agree with that approach?

@alecslupu
Copy link
Contributor Author

@alecslupu I think there are actually two things here:

1. A bug that we can't override without using !important. I think we should fix that for v0.28

2. A new feature, where we're adding specific files for overriding in system, admin and core in a separate way. This could not be backported as it's a feature.

I think it'd be cleaner to work focus first in the bug fix (case 1), backport that and then work in the feature (case 2), do you agree with that approach?

I would still consider it as a single fix. I would say that currently the decidim_application.scss allows you to target admin elements, and once the fix will be merged, we may loose the ability of customizing the admin area ( including a new bug ).
Of course, to prevent that, we could write the fix so that we properly load the decidim_application.scss in the admin area.

The bad part when you load customized CSS within admin area, is that you may start break buttons, lists etc.

To provide more context:
In order to make the Decidim feel and look like a EU Commision website, we needed to apply several customization

We customized the frontend like this adding our overrides in decidim_application.scss:
image

We got the admin like this:
image

We got the system panel like this:
image

@andreslucena andreslucena marked this pull request as draft April 3, 2024 09:19
github-actions[bot]
github-actions bot previously approved these changes Apr 4, 2024
@alecslupu alecslupu marked this pull request as ready for review April 4, 2024 20:04
@andreslucena andreslucena changed the title Fix CSS dependencies Fix CSS overrides in applications Apr 8, 2024
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Tried it locally and it works as expected:

Before

Screenshot of the web development tools showing the bug

After

Screenshot showing the fix

Apart from the small changes that I'm suggesting, can you add some notes in the Releases Notes to communicate to implementers that this new way of overriding admin/system/design is available 🙏🏽 ?

Co-authored-by: Andrés Pereira de Lucena <[email protected]>
github-actions[bot]
github-actions bot previously approved these changes Apr 8, 2024
Co-authored-by: Andrés Pereira de Lucena <[email protected]>
github-actions[bot]
github-actions bot previously approved these changes Apr 8, 2024
github-actions[bot]
github-actions bot previously approved these changes Apr 8, 2024
github-actions[bot]
github-actions bot previously approved these changes Apr 8, 2024
@alecslupu
Copy link
Contributor Author

Can you add some notes in the Releases Notes to communicate to implementers that this new way of overriding admin/system/design is available 🙏🏽 ?

Done in: b82f9a6

RELEASE_NOTES.md Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
Co-authored-by: Andrés Pereira de Lucena <[email protected]>
@andreslucena andreslucena merged commit 1132354 into develop Apr 16, 2024
110 checks passed
@andreslucena andreslucena deleted the fix/css-dependencies branch April 16, 2024 07:36
@andreslucena
Copy link
Member

Adding the no-backport label as we will not backport this to v0.27: the append_stylesheet_pack_tag isn't available in the version of webpacker that we have in this version of Decidim (v6.0.0-rc.5) as it was introduced in shakapacker v6.5.1 https://github.com/shakacode/shakapacker/blame/f902e186b102a45273d6dad4c6affb9da285763a/CHANGELOG.md#L167

@andreslucena andreslucena added the no-backport Pull Requests that should not be backported label Apr 30, 2024
andreslucena added a commit that referenced this pull request May 9, 2024
* Declare new override entrypoint for frontend

* Declare new override entrypoint for admin

* Declare new override entrypoint for system panel

* Add documentation

* Add overrides in the design app to facilitate the development of new themes

* Apply suggestions from code review

Co-authored-by: Andrés Pereira de Lucena <[email protected]>

* Apply suggestions from code review

Co-authored-by: Andrés Pereira de Lucena <[email protected]>

* Add docs

* Add Readme

* Lint

* Apply suggestions from code review

Co-authored-by: Andrés Pereira de Lucena <[email protected]>

---------

Co-authored-by: Andrés Pereira de Lucena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Pull Requests that should not be backported type: fix PRs that implement a fix for a bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

CSS overrides do not work as intended
2 participants