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

Improve theme-color-level() using abs() #24074

Merged
merged 4 commits into from
Sep 26, 2017
Merged

Improve theme-color-level() using abs() #24074

merged 4 commits into from
Sep 26, 2017

Conversation

schwastek
Copy link
Contributor

Concerns function theme-color-level() in _functions.scss file.

Use internal Sass function abs() to get the absolute value of $level argument.

That way, if/else statement can be completely eliminated.

Screenshot

Both - old and new functions - return the same values.

new theme-color-level function values

Test

Check out this pull request locally or reproduce it from scratch:

  1. Run:
git clone https://github.com/twbs/bootstrap.git
cd bootstrap
# remove all files but `scss` and `.git` folders
find -not \( \( -path ./.git -o -path ./scss \) -prune \) -exec rm -rf {} +
touch ./scss/_debug.scss
printf "@import 'functions';\n@import 'variables';\n@import 'debug';" > ./scss/bootstrap.scss
npm init --yes
npm install nodemon node-sass --save-dev
  1. Overwrite scripts section in package.json with:
"scripts": {
    "css-compile": "node-sass ./scss/bootstrap.scss ./dist/css",
    "watch-css": "nodemon --ext scss --watch ./scss --exec \"npm run css-compile\""
  }
  1. Add at the end of _functions.scss:
// _functions.scss
(...)
@function new-theme-color-level($color-name: "primary", $level: 0) {
  $color: theme-color($color-name);
  $color-base: if($level > 0, #000, #fff);
  $level: abs($level);
  
  @return mix($color-base, $color, $level * $theme-color-interval);
}
  1. Paste into _debug.scss:
@debug theme-color-level("primary", -1);
@debug theme-color-level("primary", 0);
@debug theme-color-level("primary", 1);

@debug new-theme-color-level("primary", -1);
@debug new-theme-color-level("primary", 0);
@debug new-theme-color-level("primary", 1);
  1. Run:
npm run css-compile

Improve `theme-color-level()` using `abs()`.
`abs()` gets the absolute value of `$level`.
That way, `if/else` statement can be completely eliminated.
@return mix($color-base, $color, $level * $theme-color-interval);
}
$level: abs($level);

Choose a reason for hiding this comment

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

Line contains trailing whitespace

Copy link
Member

Choose a reason for hiding this comment

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

@schwastek: please fix the lint errors.

@XhmikosR
Copy link
Member

/CC @andresgalante

@andresgalante
Copy link
Collaborator

@XhmikosR I'll build this one and test it out but I can't be definitive about it because I am not great with scss functions.

Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

I think it totally makes sense to use abs here, but please someone else confirm too.

@XhmikosR XhmikosR requested a review from mdo September 26, 2017 07:24
@andresgalante
Copy link
Collaborator

I've pull @schwastek branch, build it locally and tested changing the themes maps on variables. It works and compiles without errors.

As I mention before I can't say for sure, but to me the absolut method makes sense.

@schwastek great work! 👍

@XhmikosR XhmikosR merged commit 17fd2c9 into twbs:v4-dev Sep 26, 2017
@mdo mdo mentioned this pull request Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants