Skip to content

Centralize Sass variables into single file#1693

Merged
20wildmanj merged 7 commits into
masterfrom
joeywildman-css-variables
Jan 22, 2023
Merged

Centralize Sass variables into single file#1693
20wildmanj merged 7 commits into
masterfrom
joeywildman-css-variables

Conversation

@20wildmanj

@20wildmanj 20wildmanj commented Jan 20, 2023

Copy link
Copy Markdown
Contributor

Move Sass variables into _variables.scss in order to centralize variables into a file that can be imported into other scss / css files, without having duplicating css in production from importing. Duplicating css code was introduced after #1649 and subsequently #1657. Other features also duplicated the variables in separate CSS files.

Solution derived from the following post.

Note: The recommended option suggested to import variables in Sass is to use the @use '<file>' syntax, but sass-rails which is now deprecated doesn't support this functionality. We should probably move to dart-sass which is still actively being supported.

Description

  • Create _variables.scss to hold application-wide sass (don't use application.scss as it imports a ton of CSS from materialize and bootstrap)
  • Remove import style.css.scss from usersTable.css.scss
  • replace duplicated variables from github integration, metrics stylesheets with imported variable

How Has This Been Tested?

  • run autolab on a production environment (autolab-docker for example)
  • check public/assets/usersTable-.css, see that it only contains css from the original file and doesn't have extra style duplicated
  • can also run RAILS_ENV=development bundle exec rake assets:precompile on local development server and check the above

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting

dependabot Bot and others added 4 commits January 19, 2023 00:53
Bumps [rack](https://github.com/rack/rack) from 2.2.4 to 2.2.6.2.
- [Release notes](https://github.com/rack/rack/releases)
- [Changelog](https://github.com/rack/rack/blob/main/CHANGELOG.md)
- [Commits](rack/rack@2.2.4...v2.2.6.2)

---
updated-dependencies:
- dependency-name: rack
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@20wildmanj 20wildmanj requested a review from damianhxy January 20, 2023 03:58
@20wildmanj 20wildmanj changed the title Centralize SCSS variables into single file Centralize SASS variables into single file Jan 20, 2023
@20wildmanj 20wildmanj changed the title Centralize SASS variables into single file Centralize Sass variables into single file Jan 20, 2023

@damianhxy damianhxy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested with precompile method, LGTM

Comment thread app/assets/stylesheets/_variables.scss
Comment thread app/assets/stylesheets/_variables.scss
Comment thread app/assets/stylesheets/beta.scss Outdated

@damianhxy damianhxy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than the one missing style, LGTM

@20wildmanj 20wildmanj merged commit abe6a67 into master Jan 22, 2023
@20wildmanj 20wildmanj deleted the joeywildman-css-variables branch January 22, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants