feat(sass): Added Support for Less to Sass Conversion #692

dtaylor113 · 2017-12-13T15:50:44Z

Description

From the updated README.md:

Less to Sass Conversion

During the build process Less files are converted to Sass files in /dist/sass. Then the Sass files are compiled into /dist/sass/angular-patternfly.css. If you would like to copy the Sass generated css into the main /dist/styles directory, execute:

grunt build --sass

This task will copy /dist/sass/angular-patternfly.css to /dist/styles/angular-patternfly.css. Then the build process will minimize the css in /dist/styles.

The Less to Sass Conversion step will be accomplished and managed as a part of any Pull Request which includes Less file changes. Although contributors may want to build and test their style changes with Sass before submitting a Pull Request, this step should always be tested and validated by reviewers before a style change is merged and released. If a contributor is having issues with Sass conversion that they cannot resolve, Pull Request reviewers will need to ensure that the Sass conversion step is successfully accomplished, tested, and included in the Pull Request before it is approved and merged.

For more detailed information, please read PatternFly Less to Sass Conversion

Note: When a Less file is added/deleted/renamed it needs to be updated in the main Less import file /styles/angular-patternfly.less and the main Sass import file /styles/_angular-patternfly.scss.

New `dist/sass` directory and files.

Fixes: #688

The sass-2-css file is almost identical to the less-2-css file; just diffs in line breaks. The minified versions are identical!

**Thanks to @TheRealJon for leading the way in the less-2-sass conversion process :-)

TheRealJon · 2017-12-13T18:43:53Z

Do we want a top level _angular-patternfly.scss file to import the rest of the partials?

dtaylor113 · 2017-12-13T19:18:51Z

Do we want a top level _angular-patternfly.scss file to import the rest of the partials?

Do you mean in addition to styles/build.scss?

dtaylor113 · 2017-12-13T19:22:46Z

Don't know why Travis is failing with:


Running "sass:patternfly" (sass) task
>> 
Error: File to import not found or unreadable: ../node_modules/patternfly/dist/sass/patternfly/_color-variables.scss.
>> 
       Parent style sheet: /home/travis/build/patternfly/angular-patternfly/styles/build.scss
>> 
        on line 1 of styles/build.scss
>> 
>> @import '../node_modules/patternfly/dist/sass/patternfly/_color-variables.scss
>> 
   ^

sass:patternfly runs fine locally. package-lock.json has patternfly at v3.31.2 which should have the ../node_modules/patternfly/dist/sass/patternfly/_color-variables.scss file.

TheRealJon · 2017-12-13T19:28:31Z

Do we want a top level _angular-patternfly.scss file to import the rest of the partials?

Do you mean in addition to styles/build.scss?

Yes. The build.scss file is for dev/test purposes to build css from the sass (since sass ignores partials by default, you can't compile from a file that starts with _). The _angular-patternfly.scss partial would ship in dist so that it could be consumed. That way consumers don't have to import individual partials. Then build.scss could also just @import 'angular-patternfly';.

dtaylor113 · 2017-12-13T19:59:07Z

The _angular-patternfly.scss partial would ship in dist so that it could be consumed. That way consumers don't have to import individual partials.

Ok, implemented this. May need a little help documenting how app devs are supposed to consume _angular-patternfly.scss. -thanks

TheRealJon · 2017-12-13T20:24:44Z

@dtaylor113 I just opened a PR on your fork to flatten the dist/sass directory. I also slightly adjusted the includePaths option in your grunt sass task so that the imports in _angular-patternfly.scss can be relative. This was mainly for consistency with the way patternfly core works.

As far as downstream consumption, in most use cases, people should be able to just add node_modules/angular-patternfly/dist/sass to their build tool's search path, then @import 'angular-patternfly'; in their sass.

dtaylor113 · 2017-12-13T21:22:42Z

Awesome! Thanks @TheRealJon !!!

dtaylor113 · 2017-12-13T21:26:10Z

Arrrrg...Travis still failing with:

Running "sass:patternfly" (sass) task
>> Error: File to import not found or unreadable: patternfly/color-variables.
>>        Parent style sheet: /home/travis/build/patternfly/angular-patternfly/styles/_angular-patternfly.scss
>>         on line 1 of styles/_angular-patternfly.scss
>> >> @import 'patternfly/color-variables';
>>    ^

TheRealJon · 2017-12-13T21:43:29Z

Yeah, I just noticed this. I don't know why. The build runs fine on my machine. I'll fiddle around with it and see if I can figure something out.

dtaylor113 · 2017-12-13T22:12:37Z

Thanks @TheRealJon the switch to "patternfly": "^3.31.0" did the trick!!!!

TheRealJon · 2017-12-13T22:16:37Z

styles/build.scss

@@ -0,0 +1 @@
+@import 'angular-patternfly.scss';


@dtaylor113 can you remove the .scss extension from this import statement so that all of the imports are standardized?

jeff-phillips-18 · 2017-12-14T16:49:52Z

eslint.yaml

@@ -11,7 +11,7 @@ globals:
 rules:
  strict: [2, "function"]
  quotes: 0
-  camelcase: 2
+  camelcase: 1


Is there a reason for this change?

Yes, was throwing an error due to new task in gruntfile 'convert_within_custom_replacements' not being camelcase:

lessToSass: { convert_within_custom_replacements:

jeff-phillips-18 · 2017-12-14T16:55:03Z

styles/build.scss

@@ -0,0 +1 @@
+@import 'angular-patternfly';


I think this file needs to be named angular-patternfly.scss for it to be intuitive for consumption. This likely means moving the partials into a sub-directory or adding all the partials here rather than having an _angular-patternfly.scss

@TheRealJon recommended the main partial:
"Yes. The build.scss file is for dev/test purposes to build css from the sass (since sass ignores partials by default, you can't compile from a file that starts with _). The _angular-patternfly.scss partial would ship in dist so that it could be consumed. That way consumers don't have to import individual partials. Then build.scss could also just @import 'angular-patternfly';."

I think it needs to begin with a "_" in order to be @import'ed

It doesn't have to start with an underscore, but we are doing it that way in patternfly core, so I figured it would be better to keep things consistent. It's also done similarly in other libraries like bootstrap-sass and font-awesome-sass.

Just tested renaming to "angular-patternfly.scss" and css generated correctly. Just want to make sure that "...The angular-patternfly.scss partial would ship in dist so that it could be consumed. That way consumers don't have to import individual partials.." is still correct. In other words, app devs can sill consume (@import?) 'dist/angular-patternfly.scss', even though it doesn't begin with an underscore. -thanks

OK, this is fine then.

Just as a reference:

Sass Partials
Sass Imports

Sass does a bit of magic with imports and file names, so those sections definitely cleared some confusion up for me.

jeff-phillips-18 · 2017-12-14T20:56:14Z

I think this should be a feat. This is worthy of a point release.

dtaylor113 changed the title ~~chore(sass): Added Support for Less to Sass Conversion~~ [WIP] chore(sass): Added Support for Less to Sass Conversion Dec 13, 2017

dtaylor113 requested review from TheRealJon and jeff-phillips-18 December 13, 2017 18:08

dtaylor113 force-pushed the add-less-to-sass branch from 9602bd0 to 9cddafd Compare December 13, 2017 19:14

dtaylor113 force-pushed the add-less-to-sass branch from 9cddafd to 36d61ab Compare December 13, 2017 19:57

dtaylor113 force-pushed the add-less-to-sass branch from e3d0d66 to c540572 Compare December 13, 2017 21:22

dtaylor113 force-pushed the add-less-to-sass branch from c540572 to 2b2bf10 Compare December 13, 2017 22:02

dtaylor113 changed the title ~~[WIP] chore(sass): Added Support for Less to Sass Conversion~~ chore(sass): Added Support for Less to Sass Conversion Dec 13, 2017

TheRealJon reviewed Dec 13, 2017

View reviewed changes

dtaylor113 force-pushed the add-less-to-sass branch from 2b2bf10 to fe1bfc2 Compare December 13, 2017 22:19

TheRealJon previously approved these changes Dec 14, 2017

View reviewed changes

jeff-phillips-18 reviewed Dec 14, 2017

View reviewed changes

jeff-phillips-18 previously approved these changes Dec 14, 2017

View reviewed changes

feat(sass): Added Support for Less to Sass Conversion

c45f3d4

dtaylor113 dismissed stale reviews from jeff-phillips-18 and TheRealJon via c45f3d4 December 15, 2017 14:18

dtaylor113 force-pushed the add-less-to-sass branch from fe1bfc2 to c45f3d4 Compare December 15, 2017 14:18

jeff-phillips-18 changed the title ~~chore(sass): Added Support for Less to Sass Conversion~~ feat(sass): Added Support for Less to Sass Conversion Dec 15, 2017

jeff-phillips-18 approved these changes Dec 15, 2017

View reviewed changes

jeff-phillips-18 merged commit 8bd6996 into patternfly:master Dec 15, 2017

jeff-phillips-18 mentioned this pull request Jan 17, 2018

Add Less -> Sass Converter patternfly/patternfly-react#169

Closed

feat(sass): Added Support for Less to Sass Conversion #692

feat(sass): Added Support for Less to Sass Conversion #692

Uh oh!

Conversation

dtaylor113 commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Less to Sass Conversion

New dist/sass directory and files.

Uh oh!

TheRealJon commented Dec 13, 2017

Uh oh!

dtaylor113 commented Dec 13, 2017

Uh oh!

dtaylor113 commented Dec 13, 2017

Uh oh!

TheRealJon commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtaylor113 commented Dec 13, 2017

Uh oh!

TheRealJon commented Dec 13, 2017

Uh oh!

dtaylor113 commented Dec 13, 2017

Uh oh!

dtaylor113 commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheRealJon commented Dec 13, 2017

Uh oh!

dtaylor113 commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Dec 14, 2017

Uh oh!

Uh oh!

dtaylor113 commented Dec 13, 2017 •

edited

Loading

New `dist/sass` directory and files.

TheRealJon commented Dec 13, 2017 •

edited

Loading

dtaylor113 commented Dec 13, 2017 •

edited

Loading