Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

refactor(select): removed label and replaced with floating-label #2522

Merged
merged 12 commits into from
Apr 12, 2018

Conversation

moog16
Copy link
Contributor

@moog16 moog16 commented Apr 5, 2018

fixes #2130

Copy link
Contributor

@williamernest williamernest left a 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

@@ -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.
Copy link
Contributor

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-io
Copy link

codecov-io commented Apr 6, 2018

Codecov Report

Merging #2522 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/mdc-select/constants.js 100% <ø> (ø) ⬆️
packages/mdc-select/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fe967d...22abd3d. Read the comment docs.

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/
Copy link
Contributor

@kfranqueiro kfranqueiro Apr 6, 2018

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)

Copy link
Contributor

@kfranqueiro kfranqueiro left a 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.

Kenneth G. Franqueiro added 2 commits April 6, 2018 10:14
@kfranqueiro kfranqueiro dismissed their stale review April 6, 2018 14:21

Reverted Travis changes; Travis CI is running fine now

Copy link
Contributor

@kfranqueiro kfranqueiro left a 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

`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).
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@moog16 moog16 merged commit 9a9a890 into master Apr 12, 2018
@kfranqueiro kfranqueiro deleted the refactor/select/use-floating-label branch April 24, 2018 17:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Select to use floating label
4 participants