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

fix(textfield): add primary color to textfield label on focus #1820

Merged
merged 8 commits into from
Jan 17, 2018
Merged

fix(textfield): add primary color to textfield label on focus #1820

merged 8 commits into from
Jan 17, 2018

Conversation

williamernest
Copy link
Contributor

fixes: #1435

@codecov-io
Copy link

codecov-io commented Dec 21, 2017 •
edited
Loading

Codecov Report

Merging #1820 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1820   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files          84       84           
  Lines        3718     3718           
  Branches      486      486           
=======================================
  Hits         3697     3697           
  Misses         21       21

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 568fc32...96549ba. Read the comment docs.

amsheehan
amsheehan suggested changes Dec 21, 2017
View reviewed changes
Copy link
Contributor

@amsheehan amsheehan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think we can accomplish the same thing using sibling selectors without having to touch the adapter or foundation. E.g.:

.mdc-text-field__input:focus ~ .mdc-text-field__label

@williamernest williamernest force-pushed the fix/textfield-label-primary-color-missing-on-focus branch from ea39e02 to ac48a2e Compare December 21, 2017 21:47
@williamernest
Copy link
Contributor Author

@amsheehan
Changed it to apply the label color from the input focus psuedo class.

@williamernest williamernest force-pushed the fix/textfield-label-primary-color-missing-on-focus branch 3 times, most recently from 09ffccb to bef7e54 Compare December 22, 2017 18:32
acdvorak
acdvorak suggested changes Jan 2, 2018
View reviewed changes
packages/mdc-textfield/mdc-text-field.scss Outdated
@@ -237,7 +241,6 @@
position: absolute;
bottom: 20px;
width: calc(100% - #{$mdc-text-field-icon-padding});
color: $mdc-text-field-box-secondary-text;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this line need to be removed? It doesn't appear to be related to the :focused state.

Also, it looks like $mdc-text-field-box-secondary-text isn't used anywhere else, so if we really do need to remove this line, we should also delete the variable from _variables.scss.

Copy link
Contributor Author

@williamernest williamernest Jan 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default color property is set in the mdc-text-field-label.scss file. My idea was to let the label scss file determine the default values, and then override them when we need to change them based on interaction with the other elements in the mdc-text-field.scss file.

packages/mdc-textfield/mdc-text-field.scss Outdated
@@ -246,10 +249,6 @@
// when the ripple is activated behind it.
will-change: transform;

@include mdc-theme-dark(".mdc-text-field") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these lines need to be removed? They don't appear to be related to the :focused state.

@acdvorak acdvorak self-assigned this Jan 2, 2018
@williamernest williamernest force-pushed the fix/textfield-label-primary-color-missing-on-focus branch 2 times, most recently from bd8eb5e to a894da4 Compare January 3, 2018 23:07
@williamernest williamernest force-pushed the fix/textfield-label-primary-color-missing-on-focus branch from a894da4 to ea3d67e Compare January 3, 2018 23:33
@williamernest williamernest force-pushed the fix/textfield-label-primary-color-missing-on-focus branch from ea3d67e to 0812f04 Compare January 3, 2018 23:40
lynnmercier
lynnmercier suggested changes Jan 11, 2018
View reviewed changes
Copy link
Contributor

@lynnmercier lynnmercier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the new Sass color mixins here?

lynnmercier
lynnmercier approved these changes Jan 17, 2018
View reviewed changes
Copy link
Contributor

@lynnmercier lynnmercier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@williamernest williamernest merged commit 31aa288 into master Jan 17, 2018
@williamernest williamernest deleted the fix/textfield-label-primary-color-missing-on-focus branch January 17, 2018 21:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acdvorak acdvorak acdvorak requested changes

@lynnmercier lynnmercier lynnmercier approved these changes

@amsheehan amsheehan amsheehan approved these changes

Assignees

@acdvorak acdvorak

@amsheehan amsheehan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing Textfield label primary color application on focus
5 participants
@williamernest @codecov-io @acdvorak @amsheehan @lynnmercier