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

take account 'none' for box-shadow #27972

Merged
merged 3 commits into from
Jan 7, 2019
Merged

take account 'none' for box-shadow #27972

merged 3 commits into from
Jan 7, 2019

Conversation

wojtask9
Copy link
Contributor

@wojtask9 wojtask9 commented Jan 4, 2019

Fixes #26478

@wojtask9 wojtask9 requested a review from a team as a code owner January 4, 2019 11:19
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
@XhmikosR XhmikosR merged commit 42bed43 into twbs:v4-dev Jan 7, 2019
@danijelGombac
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants