Enable sass compiling in webpack for chrome styles #107

camilamercado · 2018-04-25T22:05:27Z

Converts chrome styles index.sss to index.scss
Adds sass-loader to webpack development and production files

msimmer

Suggesting a little bit of clean up here :)

msimmer · 2018-04-25T22:11:31Z

packages/b-ber-reader/src/index.scss

+    }
+}
+
+@keyframes spin {


Is this valid SCSS?

msimmer · 2018-04-25T22:13:19Z

packages/b-ber-reader/src/index.scss

+    border-bottom: 1px solid $black;
+    height: $component-x;
+
+    & nav ul {


The ampersands that aren't joining declarations aren't necessary, and sort of confusing I think in SCSS if they're not needed. Might be good to get rid of them

msimmer · 2018-04-25T22:14:38Z

packages/b-ber-reader/webpack.config.development.js

            exclude: /(node_modules|public|dist|test)/,
            use: ExtractTextPlugin.extract({
                fallback: 'style-loader',
                use: [
-                    {loader: 'css-loader'},
-                    {loader: 'postcss-loader'},
+                     {loader: 'css-loader', options: {minimize: true}},


Not sure we need to minify in dev. Also that space before the first curly is freaking me out 😱

msimmer · 2018-04-25T22:16:31Z

packages/b-ber-reader/package.json

@@ -47,12 +47,14 @@
    "file-loader": "^1.1.11",
    "html-webpack-plugin": "^3.0.6",
    "jest": "^22.4.3",
+    "node-sass": "^4.8.3",
    "nodemon": "^1.17.1",
    "postcss": "^6.0.19",
    "postcss-cssnext": "^3.1.0",
    "postcss-import": "^11.1.0",
    "postcss-loader": "^2.1.1",


We still need this?

looks like node-sass was removed as a peerDependency?
https://github.com/webpack-contrib/sass-loader/pull/533/files
see last comment on this issue: webpack-contrib/sass-loader#532

I was referring to postcss-loader on line 55.

Would be good to check all the postcss stuff, too, since I think we're only using autoprefixer?

@msimmer yeah the postcss dependencies aren't necessary. There are a handful of other node modules that aren't being used. Although I'm not sure if you have plans to implement some of these in the future. Also note that depcheck has a persistent issue evaluating webpack and babel deps (despite use of special flags).

camilas-MBP-2:b-ber-reader camila$ depcheck --specials=babel,webpack,eslint Unused dependencies * babel-polyfill * material-design-icons Unused devDependencies * babel-cli * babel-core * babel-loader * babel-preset-stage-0 * concurrently * css-loader * eslint-config-airbnb-base * file-loader * jest * node-sass * nodemon * postcss * postcss-cssnext * postcss-import * postcss-loader * rimraf * sass-loader * style-loader * sugarss * url-loader * webpack-cleanup-plugin * webpack-cli * webpack-dashboard * webpack-node-externals

FTM im just going to remove

* postcss * postcss-cssnext * postcss-import * postcss-loader ... * sugarss

yeah the postcss dependencies aren't necessary

Are you sure none of them are necessary? I'm fairly certain that Autoprefixer is bundled in postcss, which is being used (I think! If not then it should be).

There are a handful of other node modules that aren't being used

Did you look at that list of modules? node-sass is on it ...

Also note that depcheck has a persistent issue evaluating webpack and babel deps (despite use of special flags).

Then why use it?

msimmer · 2018-04-25T22:18:05Z

packages/b-ber-reader/package.json

    "nodemon": "^1.17.1",
    "postcss": "^6.0.19",
    "postcss-cssnext": "^3.1.0",
    "postcss-import": "^11.1.0",
    "postcss-loader": "^2.1.1",
    "rimraf": "^2.6.2",
+    "sass-loader": "^7.0.1",
    "style-loader": "^0.20.2",
    "sugarss": "^1.0.1",


This we don't need

To clarify, this refers to line 59 (sugarss). I'm adding comments here at the specific line they relate to

msimmer · 2018-04-25T22:21:04Z

packages/b-ber-reader/src/index.sss

@@ -209,4 +209,3 @@ button
        border-left-color: var(--black)


Is index.sss still in the package? Can be removed if so

msimmer · 2018-04-25T22:23:53Z

packages/b-ber-reader/webpack.config.development.js

-                    {loader: 'css-loader'},
-                    {loader: 'postcss-loader'},
+                     {loader: 'css-loader', options: {minimize: true}},
+                    {loader: 'sass-loader'}


Um, I know this sounds totally neurotic, but would be great to have a dangling comma there ...

dangling comma where?
like so? {loader: 'sass-loader'},

Yeah. https://eslint.org/docs/3.0.0/rules/comma-dangle#require-or-disallow-trailing-commas-comma-dangle

msimmer · 2018-04-25T22:24:15Z

packages/b-ber-reader/webpack.config.production.js

-                    {loader: 'css-loader'},
-                    {loader: 'postcss-loader'},
+                    {loader: 'css-loader', options: {minimize: true}},
+                    {loader: 'sass-loader'}


Same re: comma.

camilamercado · 2018-04-25T22:34:34Z

@msimmer thanks- have to run now, will revisit later

msimmer

Mostly nitpicking

msimmer · 2018-04-25T22:28:35Z

packages/b-ber-themes/b-ber-theme-crimson/package.json

+    "access": "public"
+  },
+  "scripts": {
+    "start": "sass -l --trace --watch application.scss:demo/application.css"


Might be good to be running this with node-sass instead

msimmer · 2018-04-25T22:30:23Z

packages/b-ber-themes/b-ber-theme-crimson/layout/base/_figure.scss

+
+
+    &.figure__small--portrait-high,    // new class names
+    &.portraitLong                      // legacy


We can get rid of the legacy stuff I think

msimmer · 2018-04-25T22:32:53Z

packages/b-ber-themes/b-ber-theme-crimson/layout/_layout.scss

+@import 'base/table';
+@import 'base/text';
+@import 'base/web'; // conditional styles
+@import 'base/print'; // for print views and PDF generation


Ah, I added a PR in the GitLab repo with Reader styles, looks like it got merged in, but don't see the import here? Not sure if you were planning on including those styles here yet, if not then ignore this

msimmer · 2018-04-25T22:34:16Z

packages/b-ber-themes/b-ber-theme-crimson/application.scss

+
+@if $build == 'web' {
+    .publication__contents {
+		font-size: 80%; // 20px


Are those tabs?

msimmer · 2018-04-25T22:35:31Z

packages/b-ber-themes/b-ber-theme-crimson/_settings.scss

+$grey: #eee                                                 !default;
+$dimgrey: #696969                                           !default;
+$info: #26A4B7                                              !default;
+$warning: #DA4539                                          	!default;


msimmer · 2018-04-25T22:35:55Z

packages/b-ber-themes/b-ber-theme-crimson/_settings.scss

+$info: #26A4B7                                              !default;
+$warning: #DA4539                                          	!default;
+$danger: #CD8D59                                            !default;
+$success: #5050C5                                          	!default;


That's def a tab

camilamercado · 2018-04-26T01:47:42Z

@msimmer addressed comments on webpack files and index.sss in commit 567c2512. Will revisit scss and dependencies when I have more time (later this week).

camilamercado · 2018-05-06T20:55:02Z

packages/b-ber-reader/src/index.scss

+}
+
+.material-icons {
+    font-family: 'Material Icons';


@msimmer Doing a cleanup of SCSS now with linter. Frequent error arising for Property Sort Order
PropertySortOrder: Properties should be ordered direction

Should I follow this protocol order properties alphabetically?
It seems a little counter intuitive to separate properties such as
-webkit-font-smoothing: antialiased; and -moz-osx-font-smoothing: grayscale;

I haven't set up SCSS linting WRT to style, I'm only using it to detect critical errors. If you'd like to make one up, maybe easiest is to use the current conventions as a base, and then just make up a config file where everything that's in there passes with the current SCSS.

camilamercado · 2018-05-06T21:07:22Z

packages/b-ber-reader/src/index.scss

+        }
+        & li, & dt {
+            padding: calc(#{$component-x} / 2) 0 0 calc(#{$component-x} / 2);
+            & a {


@msimmer additionally- frequent errors on SelectorDepth rule.

SelectorDepth: Selector should have depth of applicability no greater than 3, but was 4

10+ instances of this error, with selectorDepth ranging from 4-5.
Should I adapt the SCSS to stick to the default depth (3)?

Please see above response. The initial note was just about readability of the SCSS WRT ampersands, so if that's fixed then I'm happy :)

@msimmer yes this is fixed- ampersands removed and tabs converted to spaces with commit ae01c95

@msimmer unless i missed something it looks like all of the other reviews are now outdated?

camilamercado · 2018-05-08T15:44:24Z

packages/b-ber-reader/webpack.config.production.js

+              			    	require('cssnano')(),
+              			    ]
+              			  }
+              			},
                    {loader: 'sass-loader'},
                ],
            }),


@msimmer just tested this config, which is now outputting minified css w vendor prefixes and functioning in the bowser/reader.

postcss was initially throwing syntax errors when used w sass-loader, getting caught up on Unnecessary curly bracket and Unnecessary semicolon when building the index.scss file. Specifying the autoprefixer and cssnano plugins resolved these errs.

msimmer · 2018-05-08T22:17:21Z

Hey @camilamercado, can you please just squash all these changes along with the previous PR and resubmit? It's getting difficult to tell what the current state of things is right now :/

…webpack for sass build

camilamercado added 3 commits April 25, 2018 11:37

add new theme b-ber-theme-crimson

3db6aeb

rename b-ber-theme-crimson to tc-theme-crimson

b44a718

update crimson theme name in package.json

a5da50d

msimmer requested changes Apr 25, 2018

View reviewed changes

msimmer mentioned this pull request Apr 25, 2018

add new theme b-ber-theme-crimson #106

Closed

camilamercado commented May 6, 2018

View reviewed changes

camilamercado commented May 8, 2018

View reviewed changes

squashes 8 commits: converts chrome styles from cs4 to sass, updates …

a31c48c

…webpack for sass build

camilamercado force-pushed the reader-styles-dev branch from d47de14 to a31c48c Compare May 8, 2018 23:42

msimmer closed this Jun 13, 2018

camilamercado deleted the reader-styles-dev branch November 8, 2018 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable sass compiling in webpack for chrome styles #107

Enable sass compiling in webpack for chrome styles #107

camilamercado commented Apr 25, 2018

msimmer left a comment

msimmer Apr 25, 2018

msimmer Apr 25, 2018

msimmer Apr 25, 2018

msimmer Apr 25, 2018

camilamercado Apr 26, 2018

msimmer Apr 26, 2018

camilamercado May 6, 2018

msimmer May 7, 2018

msimmer Apr 25, 2018

msimmer Apr 26, 2018

msimmer Apr 25, 2018

msimmer Apr 25, 2018

camilamercado Apr 25, 2018

msimmer Apr 25, 2018

msimmer Apr 25, 2018

camilamercado commented Apr 25, 2018

msimmer left a comment

msimmer Apr 25, 2018

msimmer Apr 25, 2018

msimmer Apr 25, 2018

msimmer Apr 25, 2018

msimmer Apr 25, 2018

msimmer Apr 25, 2018

camilamercado commented Apr 26, 2018 •

edited

Loading

camilamercado May 6, 2018

msimmer May 7, 2018

camilamercado May 6, 2018

msimmer May 7, 2018

camilamercado May 7, 2018

camilamercado May 7, 2018

camilamercado May 8, 2018

msimmer commented May 8, 2018



		&.figure__small--portrait-high, // new class names
		&.portraitLong // legacy

Enable sass compiling in webpack for chrome styles #107

Enable sass compiling in webpack for chrome styles #107

Conversation

camilamercado commented Apr 25, 2018

msimmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamercado commented Apr 25, 2018

msimmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamercado commented Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimmer commented May 8, 2018

camilamercado commented Apr 26, 2018 •

edited

Loading