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

Amend CSS overwritting #8007

Merged
merged 4 commits into from May 25, 2021
Merged

Amend CSS overwritting #8007

merged 4 commits into from May 25, 2021

Conversation

ferblape
Copy link
Contributor

🎩 What? Why?

This PR improves how CSS are overwritten:

  • _decidim-settings.scss has been restored to allow redefining CSS vars and Foundation styles. The file is included after CSS vars are defined to allow easily override them
  • this file has also been included in the generator to be created in the apps for simplicity
  • docs have been improved both in the comments and the migration guides

📌 Related Issues

Testing

Create a new development_app with no CSS overrides.

Three scenarios:

  • override a CSS variable in app/packs/stylesheets/decidim/_decidim-settings.scss
$body-font-family: Consolas, "Liberation Mono", Courier, monospace;

You should see the body has changed in the whole app

  • override some CSS in app/packs/stylesheets/decidim/decidim_application.scss, i.e
p { color: #f00; }

You should paragraphs in red

  • override a whole CSS file from decidim, i.e cp decidim-core/app/packs/stylesheets/decidim/layouts/_home.scss development_app/app/packs/stylesheets/decidim/layouts/. Then edit the overriden file and change some styles, you should see those styles in the app

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@ferblape ferblape requested a review from mrcasals May 12, 2021 13:19
copy_file "decidim_application.scss", "app/packs/stylesheets/decidim/decidim_application.scss"
copy_file "_decidim-settings.scss", "app/packs/stylesheets/decidim/_decidim-settings.scss"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should copy this file to the application by default.

I would be personally fine with that (since it's needed in all cases for us) but the current core does not automatically copy this file, which is why I wonder if it should be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I was copying it for simplicity for developers to start overriding vars (I think most of the Rails apps use this feature to change colors). What do you think @mrcasals ?

@ferblape ferblape added this to Technical Review in Online Meetings May 18, 2021
Copy link
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

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

LGTM!

@leio10 leio10 merged commit 00bad01 into develop May 25, 2021
@leio10 leio10 deleted the fix/amend-css-overwriting branch May 25, 2021 10:07
@ferblape ferblape moved this from Technical Review to Done in Online Meetings May 25, 2021
entantoencuanto added a commit that referenced this pull request May 31, 2021
* develop: (59 commits)
  Update supported versions in docs (#8079)
  Meetings merge minutes and close actions (#7968)
  Meeting calendars providers (#7944)
  Fix broken test on meetings after merging PR without rebase (#8076)
  Show participants list in meetings (#7933)
  Security feature external link warning (#7397)
  Add missing tests for scope types admin page (#8053)
  Use symbols for polymorphic route arguments (#8052)
  Mockup design for Participation statistics tables in Votings (#7879)
  Fix boolean fields for .reported? and .hidden? which is nil if no report exists (#7990)
  Fix redirects broken by Terms and Conditions redirect (#8036)
  Amend CSS overwritting (#8007)
  New Crowdin updates (#8048)
  Fix undetected broken tests because of missing dependencies (#8050)
  Validate results by Monitoring Committee Members (#7899)
  Electoral certificate validation by Monitoring Committee Members (#7871)
  Publish and unpublish a meeting (#7893)
  New Crowdin updates (#8005)
  Polling station closure attach the physical electoral closure certificate (#7929)
  Fix attachment title migration generating possibly invalid values (#8020)
  ...
@andreslucena andreslucena added type: internal PRs that aren't necessary to add to the CHANGELOG for implementers and removed target: developer-experience labels Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review module: generators type: internal PRs that aren't necessary to add to the CHANGELOG for implementers
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Improvements in Webpack overwrites
4 participants