Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix CSS overrides in applications#12646

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
andreslucena merged 14 commits intodevelopfromfix/css-dependencies
Apr 16, 2024
Merged

Conversation

alecslupu
Copy link
Member

@alecslupualecslupu commentedMar 23, 2024•
edited
Loading

🎩 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, thedecidim_application.scss is being imported as part ofdecidim_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 anddecidim_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 bedisplay: block

♥️ Thank you!

@alecslupualecslupu added the type: fixPRs that implement a fix for a bug labelMar 23, 2024
github-actions[bot]
github-actionsbot previously approved these changesMar 23, 2024
github-actions[bot]
github-actionsbot previously approved these changesMar 23, 2024
github-actions[bot]
github-actionsbot previously approved these changesMar 23, 2024
@alecslupualecslupu marked this pull request as ready for reviewMarch 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
MemberAuthor

@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.282. 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 thedecidim_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 thedecidim_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 indecidim_application.scss:
image

We got the admin like this:
image

We got the system panel like this:
image

andreslucena reacted with thumbs up emoji

@andreslucenaandreslucena marked this pull request as draftApril 3, 2024 09:19
github-actions[bot]
github-actionsbot previously approved these changesApr 4, 2024
@alecslupualecslupu marked this pull request as ready for reviewApril 4, 2024 20:04
@andreslucenaandreslucena changed the titleFix CSS dependenciesFix CSS overrides in applicationsApr 8, 2024
Copy link
Member

@andreslucenaandreslucena 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 🙏🏽 ?

alecslupu reacted with thumbs up emoji
Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
github-actions[bot]
github-actionsbot previously approved these changesApr 8, 2024
Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
github-actions[bot]
github-actionsbot previously approved these changesApr 8, 2024
github-actions[bot]
github-actionsbot previously approved these changesApr 8, 2024
github-actions[bot]
github-actionsbot previously approved these changesApr 8, 2024
@alecslupu
Copy link
MemberAuthor

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

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@andreslucena
Copy link
Member

Adding theno-backport label as we will not backport this to v0.27: theappend_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.1https://github.com/shakacode/shakapacker/blame/f902e186b102a45273d6dad4c6affb9da285763a/CHANGELOG.md#L167

@andreslucenaandreslucena added the no-backportPull Requests that should not be backported labelApr 30, 2024
andreslucena added a commit that referenced this pull requestMay 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 reviewCo-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>* Apply suggestions from code reviewCo-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>* Add docs* Add Readme* Lint* Apply suggestions from code reviewCo-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>---------Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@andreslucenaandreslucenaandreslucena approved these changes

@github-actionsgithub-actions[bot]github-actions[bot] approved these changes

Assignees

@andreslucenaandreslucena

Labels
no-backportPull Requests that should not be backportedtype: fixPRs that implement a fix for a bug
Projects
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CSS overrides do not work as intended
2 participants
@alecslupu@andreslucena
[8]ページ先頭
©2009-2025 Movatter.jp