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

Add CSS postprocessor to support variables #53

Merged
merged 6 commits into from Oct 26, 2018

Conversation

zolhorvath
Copy link
Contributor

@zolhorvath zolhorvath commented Oct 18, 2018

Why (Motivation)

To speed up (and simplify) development I think that having a CSS preprocessor(#52) or a CSS postprocessor(#53) would be useful, especially for storing main variables like colors, dimensions (e.g. input-border width, border radius).

This PR adds a PostCSS-based postprocessor for this project.

Technical summary

Basics

  • Stylesheets in ./css/ are copied to ./source/css/. At the end of the processing, these are compiled into the ./dist-css/ directory.
  • Seven's own library paths are changed to use the processed CSS files.
  • The legacy ./css directory is kept to make further git merges as easy as possible.
    If a new CSS file gets in by a merge, it should be copied to ./source/css/ and it's reference should be changed in the .info.yml or in the .libraries.yml.

Scaffolding

New CSS scaffolding tasks are following the logic of the js tasks:

  • yarn build:css:
    Process sources without source maps.
  • yarn build:css-dev:
    Process sources with (external) source maps.
  • yarn watch:css:
    Watches source assets and applies distributive task if any of them changes.
  • yarn watch:css-dev:
    Watches source assets and applies development task if any of them changes.

Miscellaneous Notes

  • CSS postprocessing:
    Current config supports less browsers than the stylelint config:
    • firefox >= 16 instead of firefox >= 5
    • opera >= 15 instead of opera >= 12
      These where added to package.json.

Workarounds

core/misc should be symlinked two level above this project's folder.
For instance, if this theme is placed to [docroot]/themes/custom/seven, then [docroot]/core/misc should be symlinked to [docroot]/themes/misc.

@kiran-kadam911
Copy link
Contributor

kiran-kadam911 commented Oct 19, 2018

@zolhorvath Please add at least one reviewer to get your code review and merge.

@zolhorvath
Copy link
Contributor Author

@kiran-kadam911 Sadly, I cannot add reviewers.

@zolhorvath
Copy link
Contributor Author

zolhorvath commented Oct 19, 2018

@lauriii

From #52's review:

  1. Fine

  2. The dir of the sources can be anything. But in favor of the existing image url references, I'd prefer to keep the dist folder at the same level where the legacy css directory is. What would happen if we want the processed CSS be placed to css/dist instead?

    Right now there are only 7 occurences of url(../../images/[*]) and 6 of url(../../../images/[*]) in the project. These are referencing images which are inside Seven's root. To prevent breaking them, I see two options:

    1. Change these references to url(../../../images/[*]) now, and at the end of the project change them back. The latter one is only needed if we want to keep the original CSS dir tree.
    2. (Automatically) symlink ./images to .css/images while we add the symlink to .gitignore. (so calling ln -s ../images css/images for css:build and css:watch tasks)

    I agree that your suggestion would be nicer than the current structure, but with the current solution we'll have less issues even at the beginning and at the finishing of the project and will be able to keep the legacy ./css directory clean (without the two or three new subdir like ./css/src, ./css/dist and optionally ./css/images).

    I almost forgot that Seven's using assets from core/misc as well (17 times).

  3. Okay

@lauriii
Copy link
Contributor

lauriii commented Oct 19, 2018

Thank you!

I think the option 1 sounds good 👍

@zolhorvath zolhorvath changed the title Add CSS postprocessor to support variables WIP: Add CSS postprocessor to support variables Oct 19, 2018
@zolhorvath
Copy link
Contributor Author

I'm on this :)

Zoltan Horvath added 2 commits October 24, 2018 18:34
Ignore distributed CSS
Change url references (follow new directory structure)
@zolhorvath
Copy link
Contributor Author

@lauriii Could you please re-check?

@zolhorvath zolhorvath changed the title WIP: Add CSS postprocessor to support variables Add CSS postprocessor to support variables Oct 25, 2018
package.json Outdated
@@ -13,16 +13,34 @@
"lint:js": "node ./node_modules/eslint/bin/eslint.js .",
"lint:js-passing": "node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json .",
"lint:js-stats": "node ./node_modules/eslint/bin/eslint.js --format=./scripts/js/eslint-stats-by-type.js .",
"lint:css": "stylelint \"**/*.css\"",
"lint:css-checkstyle": "stylelint \"**/*.css\" --custom-formatter ./node_modules/stylelint-checkstyle-formatter/index.js"
"lint:css": "stylelint \"css/dist/**/*.css\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we linting the compiled CSS files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to css/src/**/*.css.

Copy link
Contributor

@lauriii lauriii left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for making all the requested changes 😊

@lauriii lauriii merged commit a5d002e into drupalux:master Oct 26, 2018
@kiran-kadam911
Copy link
Contributor

@zolhorvath If we are going to use CSS-Preprocessor then we need to install an SCSS parser for PostCSS right?
If yes then currently it's missing we need to install it.

@lauriii please guide/suggest.

@lauriii
Copy link
Contributor

lauriii commented Oct 29, 2018

@kiran-kadam911 This PR already introduced a dependency to https://github.com/postcss/postcss-custom-properties which will allow us to use variables. I don't think we should introduce a dependency to SCSS parser at least now.

@kiran-kadam911
Copy link
Contributor

kiran-kadam911 commented Oct 31, 2018

@lauriii

From #52's review:

  1. Fine

  2. The dir of the sources can be anything. But in favor of the existing image url references, I'd prefer to keep the dist folder at the same level where the legacy css directory is. What would happen if we want the processed CSS be placed to css/dist instead?
    Right now there are only 7 occurences of url(../../images/[*]) and 6 of url(../../../images/[*]) in the project. These are referencing images which are inside Seven's root. To prevent breaking them, I see two options:

    1. Change these references to url(../../../images/[*]) now, and at the end of the project change them back. The latter one is only needed if we want to keep the original CSS dir tree.
    2. (Automatically) symlink ./images to .css/images while we add the symlink to .gitignore. (so calling ln -s ../images css/images for css:build and css:watch tasks)

    I agree that your suggestion would be nicer than the current structure, but with the current solution we'll have less issues even at the beginning and at the finishing of the project and will be able to keep the legacy ./css directory clean (without the two or three new subdir like ./css/src, ./css/dist and optionally ./css/images).
    I almost forgot that Seven's using assets from core/misc as well (17 times).

  3. Okay

@zolhorvath as per current theme structure for Seven's assets from core/misc we need to change (../../../../misc/) to (../../../../../misc/) to reach that location in 17 matches. Currently its breaking the path.

@zolhorvath zolhorvath deleted the feature/var-support branch November 12, 2018 09:34
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.

None yet

3 participants