`box-shadow()` mixin: allow 'null' and drop support 'none' with multiple args #30394

ysds · 2020-03-14T16:58:18Z

The box-shadow() mixin has been changed to handle none with multiple arguments in #27972 and #28004, but it should be null instead of none.

This PR make the box-shadow() mixin allow null and drop support none with multiple args. And add warning message when used none with multiple arguments.

before: https://www.sassmeister.com/gist/bdf41c41937a614861a026169e39279e
after: https://www.sassmeister.com/gist/56fdeffc72831664e546baf22f365cdf

mdo · 2020-03-17T19:23:18Z

In cases of none, none, should that be a single none?

MartijnCuppens · 2020-03-18T08:54:33Z

In cases of none, none, should that be a single none?

In cases of none, none, you're doing something wrong. Not sure if we should really try to tackle all possible errors, unless it's something which can occur by configuring the Sass variables (which is not the case)

ysds · 2020-03-18T11:48:46Z

I agree with @MartijnCuppens. I think it should output incorrect value as it is, not implicitly fixing it.

XhmikosR · 2020-03-19T07:29:42Z

site/content/docs/4.3/migration.md

@@ -42,6 +42,7 @@ Changes to our source Sass files and compiled CSS.
 - The `button-variant()` mixin now accepts 3 optional color parameters, for each button state, to override the color provided by `color-yiq()`. By default, these parameters will find which color provides more contrast against the button state's background color with `color-yiq()`.
 - The `button-outline-variant()` mixin now accepts an additional argument, `$active-color`, for setting the button's active state text color. By default, this parameter will find which color provides more contrast against the button's active background color with `color-yiq()`.
 - Ditch the Sass map merges, which makes it easier to remove redundant values. Keep in mind you now have to define all values in the Sass maps like `$theme-colors`. Check out how to deal with Sass maps on the [theming documentation]({{< docsref "/getting-started/theming#maps-and-loops" >}}).
+- The `box-shadow()` mixin now output as it is whatever when passed 'none' with multiple arguments. [See #30394](https://github.com/twbs/bootstrap/pull/30394).


This might need some wording fixes. /CC @mdo

wojtask9 · 2020-03-23T08:14:34Z

I added this none checks because I was trying to fix #26478

Looking at code
https://github.com/twbs/bootstrap/blob/master/scss/mixins/_buttons.scss#L52
it supports setting variable $btn-active-box-shadow to none.
But then this code
https://github.com/twbs/bootstrap/blob/master/scss/_buttons.scss#L38
produces invalid css rule (for example box-shadow: 1px 1px 1px black, none).

Probably with this PR checks $btn-active-box-shadow != none should be changed to $btn-active-box-shadow != null

x

MartijnCuppens · 2020-03-23T14:39:53Z

I think I'm gonna change sides here, @ysds. It's actually quite easy to autofix the situations with multiple values and null:

https://www.sassmeister.com/gist/49f9c4e91db45677f45dfd2e8d5cf397

MartijnCuppens · 2020-03-28T11:53:38Z

ping @ysds

I think we should switch to the example I suggested, it will auto-fix the issue @wojtask9 is talking about. We can also leave out the $btn-active-box-shadow != none checks in other places.

ysds · 2020-03-28T13:19:29Z

@MartijnCuppens Your sassmeister doesn't work in libsass(node-sass).

I think the box-shadow mixin should not do autofix.

box-shadow(none, 0 0 1px #000);
box-shadow(0 0 1px #000, none);

Add or remove shadow, not 100% sure which one the user intends. It should be outputted incorrect value as it is, not implicitly fixing it. So I will change the mixins/_buttons.scss.

ysds · 2020-03-28T13:28:11Z

And also, $btn-active-box-shadow: null is absolutely correct than $btn-active-box-shadow: none.

MartijnCuppens · 2020-03-28T13:56:04Z

@MartijnCuppens Your sassmeister doesn't work in libsass(node-sass).

Seems to work fine with the Bootstrap setup which is node-sass based

Add or remove shadow, not 100% sure which one the user intends.

The issue with $btn-active-box-shadow: none shows us we can run in to problems with the current config. We can of course fix this by adding an additional if test, but I'm not sure if it's a good idea to move that logic outside of the mixin. We now have to think about this every time we use the mixin, while it can easily be fixed for us.

Add or remove shadow, not 100% sure which one the user intends.

In our case, we probably get into this situation when people try to remove one of the shadows that would be set, so in both cases box-shadow: 0 0 1px #000; is probably the result they want.

ysds · 2020-03-28T15:06:09Z

Seems to work fine with the Bootstrap setup which is node-sass based

See the null1 and 4 cases.

In our case, we probably get into this situation when people try to remove one of the shadows that would be set,

Why not use $btn-active-box-shadow: null ?

ysds · 2020-03-28T15:11:33Z

In our case, we probably get into this situation when people try to remove one of the shadows that would be set,

That just in our case. The mixin should expect any cases.

MartijnCuppens · 2020-03-28T15:16:46Z

Seems to work fine with the Bootstrap setup which is node-sass based

See the null1 and 4 cases.

I did something wrong, code doesn't work in node-sass indeed.

Why not use $btn-active-box-shadow: null ?

That doesn't work, you can't unset variables with null, they will be overridden by the Bootstrap variables (which is a pity).

ysds · 2020-04-01T12:44:30Z

That doesn't work, you can't unset variables with null,

I was forgetting that... I’ll reconsider this.

… args (twbs#30394) * Support 'null' and drop `none` with multiple args * Output a warning when use 'none' with multiple arguments * Add migration note * Update migration.md Co-authored-by: Mark Otto <[email protected]> Co-authored-by: XhmikosR <[email protected]>

ysds added css v5 labels Mar 14, 2020

ysds requested a review from a team March 14, 2020 16:58

ysds changed the title ~~box-shadow() mixin allow 'null' and drop none with multiple args~~ box-shadow() mixin allow 'null' and drop support 'none' with multiple args Mar 14, 2020

MartijnCuppens previously approved these changes Mar 15, 2020

View reviewed changes

XhmikosR reviewed Mar 19, 2020

View reviewed changes

zalog mentioned this pull request Mar 29, 2020

Use box-shadow mixin for form controls #30480

Merged

mdo changed the base branch from master to main June 16, 2020 20:04

ysds added 3 commits June 18, 2020 10:11

Support 'null' and drop none with multiple args

327ccd5

Output warning when use 'none' with multiple arguments

85f56a9

Add migration note

9ad3934

XhmikosR requested review from mdo and MartijnCuppens June 18, 2020 07:11

XhmikosR force-pushed the master-ysds-box-shadow-null branch from dcb5d5f to 9ad3934 Compare June 18, 2020 07:12

Update migration.md

1d7d916

mdo approved these changes Aug 3, 2020

View reviewed changes

Merge branch 'main' into master-ysds-box-shadow-null

5bc9ba1

XhmikosR merged commit e8566e1 into main Aug 4, 2020

XhmikosR deleted the master-ysds-box-shadow-null branch August 4, 2020 18:51

XhmikosR changed the title ~~box-shadow() mixin allow 'null' and drop support 'none' with multiple args~~ box-shadow() mixin: allow 'null' and drop support 'none' with multiple args Aug 6, 2020

voltaek mentioned this pull request Dec 11, 2020

[v5] Warnings and invalid CSS properties with enabled shadows but set to none #32412

Closed

dmetzner mentioned this pull request Sep 24, 2021

Bump bootstrap from 4.6.0 to 5.1.1 Catrobat/Catroweb#2047

Merged

CloCkWeRX mentioned this pull request Jan 26, 2024

Upgrade to bootstrap 5 Growstuff/growstuff#3605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`box-shadow()` mixin: allow 'null' and drop support 'none' with multiple args #30394

`box-shadow()` mixin: allow 'null' and drop support 'none' with multiple args #30394

ysds commented Mar 14, 2020 •

edited

Loading

mdo commented Mar 17, 2020

MartijnCuppens commented Mar 18, 2020

ysds commented Mar 18, 2020

XhmikosR Mar 19, 2020

wojtask9 commented Mar 23, 2020 •

edited

Loading

MartijnCuppens commented Mar 23, 2020

MartijnCuppens commented Mar 28, 2020

ysds commented Mar 28, 2020 •

edited

Loading

ysds commented Mar 28, 2020

MartijnCuppens commented Mar 28, 2020

ysds commented Mar 28, 2020 •

edited

Loading

ysds commented Mar 28, 2020

MartijnCuppens commented Mar 28, 2020

ysds commented Apr 1, 2020 •

edited

Loading

box-shadow() mixin: allow 'null' and drop support 'none' with multiple args #30394

box-shadow() mixin: allow 'null' and drop support 'none' with multiple args #30394

Conversation

ysds commented Mar 14, 2020 • edited Loading

mdo commented Mar 17, 2020

MartijnCuppens commented Mar 18, 2020

ysds commented Mar 18, 2020

XhmikosR Mar 19, 2020

Choose a reason for hiding this comment

wojtask9 commented Mar 23, 2020 • edited Loading

MartijnCuppens commented Mar 23, 2020

MartijnCuppens commented Mar 28, 2020

ysds commented Mar 28, 2020 • edited Loading

ysds commented Mar 28, 2020

MartijnCuppens commented Mar 28, 2020

ysds commented Mar 28, 2020 • edited Loading

ysds commented Mar 28, 2020

MartijnCuppens commented Mar 28, 2020

ysds commented Apr 1, 2020 • edited Loading

`box-shadow()` mixin: allow 'null' and drop support 'none' with multiple args #30394

`box-shadow()` mixin: allow 'null' and drop support 'none' with multiple args #30394

ysds commented Mar 14, 2020 •

edited

Loading

wojtask9 commented Mar 23, 2020 •

edited

Loading

ysds commented Mar 28, 2020 •

edited

Loading

ysds commented Mar 28, 2020 •

edited

Loading

ysds commented Apr 1, 2020 •

edited

Loading