Problem/Motivation

We use global "importFrom" setting from 'postcss-custom-properties'. This doesn't work in core because this would lead into leaking our variables to the local CSS scope.

Proposed resolution

Import variables.css manually to make it specific to Claro even when PostCSS is being compiled in a larger scope.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3084189-5.claro_.Import-variablescss-manually.patch18.97 KBfhaeberle
#5 interdiff.3084189.4-5.txt1.37 KBfhaeberle
#4 interdiff.txt2.9 KBlauriii
#4 claro-postcss-import-3084189-4.patch18.97 KBlauriii
#2 claro-postcss-import-3084189-2.patch16.07 KBlauriii
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Active » Needs review
FileSize
claro-postcss-import-3084189-2.patch16.07 KB

This is where I got with this. Only downside I could tell with this approach is that the manual import leads into empty :root selector at the beginning of the file. This seems like a minor problem which we could try to address in a follow-up.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Needs review » Needs work

As I see, this patch imports variables only for some assets inside the css/src/components folder, but we use those vars elsewhere as well (layout, theme).

Assets that will miss variables after patch #2:

  1. ./css/dist/layout/node-add.css
  2. ./css/dist/layout/card-list.css
  3. ./css/dist/layout/local-actions.css
  4. ./css/dist/components/autocomplete-loading.module.css
  5. ./css/dist/components/system-admin--panel.css
  6. ./css/dist/components/jquery.ui/theme.css
  7. ./css/dist/theme/field-ui.admin.css
  8. ./css/dist/theme/filter.theme.css
  9. ./css/dist/theme/ckeditor-editor.css
  10. ./css/dist/theme/ckeditor-dialog.css

My recommendation: import base/variables.css in every asset, unconditionally.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
FileSize
claro-postcss-import-3084189-4.patch18.97 KB
interdiff.txt2.9 KB

Fixed ✌️

My recommendation: import base/variables.css in every asset, unconditionally.

Not sure which way is better. I'd keep it so that only files using variables has the import to not cause any potential for confusion. It would be also hard to enforce this in core.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
CreditAttribution: fhaeberle commented
FileSize
interdiff.3084189.4-5.txt1.37 KB
3084189-5.claro_.Import-variablescss-manually.patch18.97 KB

Reviewed this! In some places, there were still imports missing and in some places I deleted the import because they aren't any variables being used in the particular file. Leaving on needs review because of new patch.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Needs review » Reviewed & tested by the community

After applied #5 and post-processed out assets, I cannot find any var() usage in the css/dist folder anymore.

  • lauriii committed f2cbf4e on 8.x-1.x 
    Issue #3084189 by lauriii, fhaeberle, huzooka: Import variables.css...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thank you! Committed! 🚀

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.