add !default to all SCSS variable declarations #337

dmackerman · 2016-12-08T18:43:11Z

Fixes #194

Checklist

~~Include tests~~ N/A
~~Update documentation~~ N/A

Changes proposed in this pull request:

Allow all of the variables defined to be overridden .

Reviewers should focus on:

Completeness

palantirtech · 2016-12-08T18:43:12Z

Thanks for your interest in palantir/blueprint, @dmackerman! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

adidahiya · 2016-12-08T19:14:25Z

I forsee a lot of stylelint failures. Here's what our config looks like https://github.com/palantir/stylelint-config-palantir/blob/master/stylelint.config.js

giladgray

thanks for taking the time to put this together @dmackerman! looking great 💅

giladgray · 2016-12-08T19:15:49Z

packages/core/src/common/_colors.scss

+$sepia2: #7d5125 !default;
+$sepia3: #96622d !default;
+$sepia4: #b07b46 !default;
+$sepia5: #c99765 !default;


hmm i wonder if !default is correct here. these colors are carefully calibrated and we shouldn't encourage mucking with them (consider them constants). instead, if you'd like to change colors you should change the aliases like $pt-text-color.

could add a comment to this effect at the top of this file?

Agreed, wouldn't make sense to replace the value of $sepia1 with another color, it's already sepia.

giladgray · 2016-12-08T19:16:24Z

packages/core/src/common/_font-imports.scss

-@import "~bourbon/app/assets/stylesheets/bourbon";
-@import "variables";
+@import '~bourbon/app/assets/stylesheets/bourbon';
+@import 'variables';


we're all about double quotes over here. gonna fail stylelint until you revert all these quote changes.

run gulp sass-lint locally until it passes 👍

llorca · 2016-12-08T19:20:35Z

packages/core/src/common/_colors.scss

+$sepia2: #7d5125 !default;
+$sepia3: #96622d !default;
+$sepia4: #b07b46 !default;
+$sepia5: #c99765 !default;


Agreed, wouldn't make sense to replace the value of $sepia1 with another color, it's already sepia.

llorca · 2016-12-08T19:21:15Z

packages/core/src/common/_color-aliases.scss

 // Default text selection color using #7dbcff
-$pt-text-selection-color: rgba(125, 188, 255, 0.6);
+$pt-text-selection-color: rgba(125, 188, 255, .6) !default;


Decimals start with 0 for clarity, I think the linter will give you trouble with this change

llorca · 2016-12-08T19:27:34Z

Missing files so far:

button/common
tag/common
slider/common
tooltip/common
all the common files for table

dmackerman · 2016-12-08T19:45:25Z

Thanks for the comments. I just realized that I had CSSComb auto formatting. I'll clean those up! 👍

giladgray · 2016-12-08T19:54:19Z

packages/core/src/common/_colors.scss

+$gold2: #bf8c0a !default;
+$gold3: #d99e0b !default;
+$gold4: #f2b824 !default;
+$gold5: #ffc940 !default;


wait not just sepia! every single variable in this file

revert the !defaults and add a comment about changing colors: change aliases instead of these named constants.

giladgray · 2016-12-08T19:57:25Z

packages/core/src/common/_variables.scss

                 sans-serif,
-                 "Icons16"; // support inline Palantir icons
+                 'Icons16' !default; // support inline Palantir icons


should be double quotes here

giladgray · 2016-12-08T19:59:52Z

@dmackerman would you be able to enable CircleCI for your fork? should be approximately one button press (after you login to https://circleci.com). also would set you up to easily contribute more code in the future! 🎩

dmackerman · 2016-12-08T20:02:22Z

@giladgray just activated my fork on CircleCI. 👍

giladgray · 2016-12-08T20:04:09Z

packages/core/src/common/_color-aliases.scss

@@ -7,7 +7,8 @@
 Color aliases

 These variables are semantic aliases of our [colors](#colors). They are used throughout Blueprint itself to ensure
-consistent color usage across components.
+consistent color usage across components. Override these variables to change the overall look and feel for your
+application.


mm this sentence is more confusing than helpful. don't think it's necessary yet, please remove. we'll draw up more complete docs on sass consumption after this feature merges.

Yeah, I kinda felt that way when writing it. People familiar with SASS should understand that if they see a list of variables, those are overridable.

adidahiya · 2016-12-09T19:51:46Z

thanks @dmackerman

Closes palantir#194.

add !default to all variable declarations

108f217

giladgray suggested changes Dec 8, 2016

View reviewed changes

llorca suggested changes Dec 8, 2016

View reviewed changes

clean up sass-lint failures, finish up the rest of the missing variables

4a52b55

giladgray reviewed Dec 8, 2016

View reviewed changes

giladgray suggested changes Dec 8, 2016

View reviewed changes

remove !default from colors, add note about color aliases, quoting

813a20d

giladgray reviewed Dec 8, 2016

View reviewed changes

remove docs addition on variables

226eb64

adidahiya approved these changes Dec 9, 2016

View reviewed changes

giladgray approved these changes Dec 9, 2016

View reviewed changes

llorca approved these changes Dec 9, 2016

View reviewed changes

adidahiya merged commit 805c4c5 into palantir:master Dec 9, 2016

greglo pushed a commit to greglo/blueprint that referenced this pull request Dec 12, 2016

Add !default to all SCSS variable declarations (palantir#337)

c7b078b

Closes palantir#194.

isTravis mentioned this pull request Jan 3, 2017

Form Controls JS API does not document disable parameter #396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add !default to all SCSS variable declarations #337

add !default to all SCSS variable declarations #337

dmackerman commented Dec 8, 2016 •

edited by adidahiya

palantirtech commented Dec 8, 2016

adidahiya commented Dec 8, 2016

giladgray left a comment •

edited

giladgray Dec 8, 2016

giladgray Dec 8, 2016

llorca Dec 8, 2016

giladgray Dec 8, 2016

giladgray Dec 8, 2016

llorca Dec 8, 2016

llorca Dec 8, 2016

llorca commented Dec 8, 2016

dmackerman commented Dec 8, 2016

giladgray Dec 8, 2016

giladgray Dec 8, 2016

giladgray Dec 8, 2016

giladgray commented Dec 8, 2016

dmackerman commented Dec 8, 2016

giladgray Dec 8, 2016

dmackerman Dec 8, 2016

adidahiya commented Dec 9, 2016

add !default to all SCSS variable declarations #337

add !default to all SCSS variable declarations #337

Conversation

dmackerman commented Dec 8, 2016 • edited by adidahiya

Fixes #194

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

palantirtech commented Dec 8, 2016

adidahiya commented Dec 8, 2016

giladgray left a comment • edited

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

llorca commented Dec 8, 2016

dmackerman commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray commented Dec 8, 2016

dmackerman commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented Dec 9, 2016

dmackerman commented Dec 8, 2016 •

edited by adidahiya

giladgray left a comment •

edited