Skip to content

take account 'none' for box-shadow#27972

Merged
XhmikosR merged 3 commits intotwbs:v4-devfrom
wojtask9:btn_fix
Jan 7, 2019
Merged

take account 'none' for box-shadow#27972
XhmikosR merged 3 commits intotwbs:v4-devfrom
wojtask9:btn_fix

Conversation

@wojtask9
Copy link
Contributor

@wojtask9 wojtask9 commented Jan 4, 2019 •
edited by XhmikosR
Loading

Fixes #26478

@wojtask9 wojtask9 requested a review from a team as a code owner January 4, 2019 11:19
MartijnCuppens
MartijnCuppens approved these changes Jan 5, 2019
View reviewed changes
Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

I've just tested this and it LGTM. I was a bit worried the mixin couldn't handle multiple values because sass lists can be separated by as well spaces as commas (https://hugogiraudel.com/2013/07/15/understanding-sass-lists/#sass-list-fun-facts), but that doesn't seemed to be a problem

@mdo mdo mentioned this pull request Jan 7, 2019
Closed
@XhmikosR XhmikosR merged commit 42bed43 into twbs:v4-dev Jan 7, 2019
@danijelGombac
Copy link

danijelGombac commented Jan 7, 2019

@MartijnCuppens in scss/_buttons.scss

&.disabled,
  &:disabled {
    opacity: $btn-disabled-opacity;
    @include box-shadow(none); // this is not take into account
}

@wojtask9
Copy link
Contributor Author

wojtask9 commented Jan 8, 2019

indeed. I missed that in my PR.

there is another place where it breaks
https://github.com/twbs/bootstrap/blob/v4-dev/scss/_button-group.scss#L102

Possible solutnions:

  1. change this to box-shadow(unset)
  2. change mixin box-shadow in that way if there is only one none shadow then apply to box-shadow property

@wojtask9 wojtask9 mentioned this pull request Jan 8, 2019
Merged
MartijnCuppens pushed a commit that referenced this pull request Jan 12, 2019
@wojtask9 @MartijnCuppens
take account 'none' for box-shadow (#27972)
28b9ac3
@ysds ysds mentioned this pull request Mar 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MartijnCuppens MartijnCuppens MartijnCuppens approved these changes

Assignees

No one assigned

Labels

css v4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wojtask9 @danijelGombac @MartijnCuppens @XhmikosR