-
Notifications
You must be signed in to change notification settings - Fork 2.1k
refactor(select): removed label and replaced with floating-label #2522
Conversation
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.
Just a quick nit, but LGTM
packages/mdc-select/README.md
Outdated
@@ -164,12 +164,12 @@ Mixin | Description | |||
--- | --- | |||
`mdc-select-ink-color($color)` | Customizes the color of the selected item displayed in the select. | |||
`mdc-select-container-fill-color($color)` | Customizes the background color of the select. | |||
`mdc-select-label-color($color)` | Customizes the label color of the select in the unfocused state. | |||
`mdc-select-label-color($color, $opacity)` | Customizes the label color of the select in the unfocused state. |
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 indicate the default value of opacity here.
Codecov Report
@@ Coverage Diff @@
## master #2522 +/- ##
==========================================
- Coverage 98.62% 98.62% -0.01%
==========================================
Files 104 101 -3
Lines 4233 4210 -23
Branches 533 532 -1
==========================================
- Hits 4175 4152 -23
Misses 58 58
Continue to review full report at Codecov.
|
karma.conf.js
Outdated
@@ -92,6 +92,16 @@ const SL_LAUNCHERS = { | |||
// }, | |||
}; | |||
|
|||
// Added Chrome --no-sandbox option because | |||
// https://github.com/travis-ci/docs-travis-ci-com/blob/ |
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.
Shorten link to https://github.com/travis-ci/docs-travis-ci-com/blob/c1da4af0/user/chrome.md#sandboxing
so you can fit it on one line :)
Also, I think I'm a bit confused... Doesn't Travis run our unit tests via Sauce Labs? This config affects the not-Sauce-Labs case, so how does it end up fixing it?
(Edit: This most certainly affects local testing, not sure if it also affects Travis)
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.
Please revert your changes to the karma config. The Travis failure you saw indicates that it is going down the wrong branch of our karma config entirely; it should be following the Sauce Labs code path, and you are altering the local testing code path (in a way we don't want, I might add; running non-headless is useful for debugging).
Edit: I did some tests on a separate branch based on this one, and Travis ran fine; I'm reverting the travis-related commits in this branch now and it's also running fine. Not sure what happened yesterday, but I suspect it somehow didn't get its environment variables sent through correctly.
Reverted Travis changes; Travis CI is running fine now
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 forgot demos/theme/index.html
packages/mdc-select/README.md
Outdated
`mdc-select-focused-label-color($color, $opacity: 0.87)` | Customizes the label color of the select when focused. Changing opacity for the label when floating is optional. | ||
`mdc-select-bottom-line-color($color)` | Customizes the color of the default bottom line of the select. | ||
`mdc-select-focused-bottom-line-color($color)` | Customizes the color of the bottom line of the select when focused. | ||
|
||
> NOTE: To customize label color please see the [label readme](./label/README.md). | ||
> NOTE: To customize label color please see the [floating label readme](./../mdc-floating-label/README.md). |
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 a little confused here... we say to see a separate readme for customizing label color, but there are two mixins directly above that seem to cover this?
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.
Maybe it would be word better as:
NOTE: To further customize label color please see...
There is fill-color that we do not cover in the mixins above.
fixes #2130