Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(text-field): Apply error color on bottom line of fullwidth field #2197

Merged
merged 3 commits into from
Feb 8, 2018

Conversation

kfranqueiro
Copy link
Contributor

Also fixes typos in a couple of private mixins (asterix -> asterisk, full-width -> fullwidth for consistency with other mixins incl. a public one)

Observable using the required checkbox under the fullwidth examples at the end of text-field.html.

Fixes #2165.

@codecov-io
Copy link

codecov-io commented Feb 6, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2197   +/-   ##
=======================================
  Coverage   99.14%   99.14%           
=======================================
  Files          90       90           
  Lines        3842     3842           
  Branches      496      496           
=======================================
  Hits         3809     3809           
  Misses         33       33

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 667513c...f966962. Read the comment docs.

williamernest
williamernest previously approved these changes Feb 6, 2018
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.

LGTM

@williamernest williamernest dismissed their stale review February 6, 2018 17:32

Alternate Colors error incorrect

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.

Alternate colors error line should be orange

@include mdc-text-field-fullwidth_;
}

.mdc-text-field--fullwidth.mdc-text-field--invalid {
Copy link
Contributor

Choose a reason for hiding this comment

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

When the alternate colors class is added, the idle line is still red.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I would point out that the alternate color class was never doing anything to the idle bottom line color for the fullwidth text field, so this might actually be a separate issue, but I'm looking into it a bit.

Copy link
Contributor Author

@kfranqueiro kfranqueiro Feb 7, 2018

Choose a reason for hiding this comment

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

So this seems like it was more of an oversight with the demo page than with these styles. I added styles to the demo page for the full-width custom color demo, and also fixed a style for the textfield custom color demo that seemed accidentally redundant (idle/focus used same opacity).

I suspect that those styles could be further cleaned up; I initially ended up confused as to where to put the styles for this specific part of the demo page, and realized that the demo-text-field-custom-colors and demo-text-field-custom-error-colors styles probably don't actually need separate CSS classes since they're always applied together to the same elements...

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.

LGTM

@kfranqueiro kfranqueiro merged commit a6500bd into master Feb 8, 2018
@kfranqueiro kfranqueiro deleted the fix/text-field-fullwidth-error branch February 8, 2018 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Textfield - Invalid fullwidth textfields don't have red bottom border
3 participants