-
Notifications
You must be signed in to change notification settings - Fork 65
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1930 +/- ##
======================================
Coverage 100% 100%
======================================
Files 416 416
Lines 8757 8757
Branches 1292 1292
======================================
Hits 8757 8757 Continue to review full report at Codecov.
|
@@ -18,7 +18,7 @@ | |||
(mousedown)="onMouseDown($event)"> | |||
</button> | |||
|
|||
<div class="sky-flyout-header" #flyoutHeader> | |||
<div class="sky-flyout-header sky-padding-squish-large" #flyoutHeader> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing the .css declaration for sky-padding-squish-large
. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great observation! Global CSS class names are provided by @skyux/theme
. See: https://github.com/blackbaud/skyux-theme/blob/master/src/app/public/styles/_sizing.scss#L21-L23
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from what I know about everything
} | ||
|
||
.sky-grid-row { | ||
@include sky-border(row, bottom); | ||
|
||
&:nth-child(odd) { | ||
background-color: $sky-grid-odd-background-color; | ||
background-color: $sky-color-gray-01; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also contain the !default
property? I noticed the old style you're replacing had that...
Or is that a complie-time sass thing? Just wanted to ask just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, it is a build-time feature of SASS. It's usually added when assigning SASS variables. In this case, it's not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I decided to name the “internal” SCSS folder
_compat
, much like RxJS’s newrxjs-compat
library. My reasoning is that it keeps existing component style sheets as-is, but it communicates that it’s not ideal and should be removed at some point. I’m adding this to the top of component’s SCSS files:Here is the
@skyux/theme
repo: https://github.com/blackbaud/skyux-theme