[HAR-107] Add bottom box shadow to buttons #29
Conversation
|
A box-shadow for a button is very rare. I would not advise to add this to a button, even if it is an if-statement. A box-shadow will create a kind of elevation which should be determined globally. Create 'button--elevated' or for when you want to increase the box-shadow 'button--elevated-200' and use global variables. |
|
I agree this is a rare case, maybe the variable should me empty by default. I'm not sure about the modifier. When it's part of your clients identity, a modifier on every button doesn't feel right to me. An other option may be some extra layer of styling in your clients codebase. In your button.scss: @import '~aanzee-harbour/scss/controls/button';
.button {
box-shadow: 0 2px 0 $color-x;
} |
|
I believe this should never be part of a clients identity. Default elevation should always be 0. |
|
I agree that a box shadow should not be default for the button. However, I think it can be part of a client's identity when a designer decides this is the default styling for all buttons. I think in that case a modifier on all buttons is not desired. |
…and fix style lint errors
| @@ -15,37 +17,47 @@ $button-disabled-background-color: $color-ui-40 !default; | |||
| color: $button-color; | |||
| text-align: center; | |||
| font-weight: normal; | |||
| line-height: 16px; | |||
| line-height: $spacing * 2; | |||
There was a problem hiding this comment.
Line-height based on line-height variable and see if a multi-line button looks good? A vertical padding can be calculated automatically with: $control-height-100 - $font-size-x * $line-height-x / 2
Or is this a different pull request ;-)
There was a problem hiding this comment.
Agree, but lets fix this in a different pull request 🥜
scss/controls/button.scss
Outdated
|
|
||
| &:hover { | ||
| background-color: $button-primary-hover-background-color; | ||
| color: $button-primary-hover-color; | ||
| } | ||
|
|
||
| &:focus { | ||
| box-shadow: $button-primary-focus-box-shadow; | ||
| @if ($button-outset) { | ||
| box-shadow: $button-call-to-action-focus-box-shadow, $button-primary-outset-box-shadow; |
There was a problem hiding this comment.
$button-call-to-action-focus-box-shadow -> $button-primary-focus-box-shadow
scss/controls/button.scss
Outdated
|
|
||
| &:hover { | ||
| background-color: $button-secondary-hover-background-color; | ||
| color: $button-secondary-hover-color; | ||
| } | ||
|
|
||
| &:focus { | ||
| box-shadow: $button-secondary-focus-box-shadow; | ||
| @if ($button-outset) { | ||
| box-shadow: $button-call-to-action-focus-box-shadow, $button-secondary-outset-box-shadow; |
There was a problem hiding this comment.
$button-call-to-action-focus-box-shadow -> $button-secondary-focus-box-shadow
There was a problem hiding this comment.
Too much copy paste :(
scss/controls/button.scss
Outdated
|
|
||
| &:focus { | ||
| box-shadow: $button-focus-box-shadow; | ||
| @if ($button-outset) { |
There was a problem hiding this comment.
This should be an if else
scss/controls/button.scss
Outdated
|
|
||
| @include media(0, $range-palm) { | ||
| padding: 8px 24px; | ||
| padding: ($spacing) ($spacing * 3); |
There was a problem hiding this comment.
($spacing) should be $spacing
scss/controls/button.scss
Outdated
| font-size: $font-size-90; | ||
| } | ||
|
|
||
| .button--90 { | ||
| @include media(0, $range-palm) { | ||
| padding: 8px 20px; | ||
| padding: ($spacing) ($spacing * 2.5); |
There was a problem hiding this comment.
($spacing) should be $spacing
Add default box shadows to buttons. Only set the box-shadow property if the box shadow variable is not set to an empty string later on.