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

Addresses 3 issues #1053

Closed
wants to merge 0 commits into from
Closed

Addresses 3 issues #1053

wants to merge 0 commits into from

Conversation

teoxoy
Copy link

@teoxoy teoxoy commented Aug 1, 2017

#999 - Partially completed (I need answers)
#1018 - Added new feature to MDCTextfield
#1049 - Updated READMEs to fix es2015 imports
Sorry for referencing 3 issues in 1 PR.

@codecov-io
Copy link

codecov-io commented Aug 1, 2017

Codecov Report

Merging #1053 into master will decrease coverage by 0.15%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1053      +/-   ##
==========================================
- Coverage   99.93%   99.78%   -0.16%     
==========================================
  Files          68       68              
  Lines        3274     3284      +10     
  Branches      403      404       +1     
==========================================
+ Hits         3272     3277       +5     
- Misses          2        7       +5
Impacted Files Coverage Δ
packages/mdc-textfield/index.js 96.87% <0%> (-3.13%) ⬇️
packages/mdc-textfield/foundation.js 97.14% <72.72%> (-2.86%) ⬇️

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 d635327...c459b51. Read the comment docs.

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.

Thanks for cleaning up our READMEs! Lets get these es2015 import fixes in first.

Please move the Textfield.valid code to another PR. I tried to highlight the files/lines of code you need to move.

And lets talk more about labels across disabled switches, radio buttons, and checkboxes more before we get a PR out.

Thanks for fixing three things at once 😄 , it is just hard for my weary brain to review 12 files at once 😩 .

@@ -192,3 +193,11 @@
}
}
}

.mdc-switch-label {
Copy link
Contributor

Choose a reason for hiding this comment

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

Take this out of this PR. I want to discuss more if we want one mdc-label for switch, checkbox, and radio...or if we need three classes.

An input's validity is checked via `checkValidity()` on blur, and the styles are updated
accordingly. When using the `required` attribute, an asterisk will be automatically appended to the
label text, as per the spec.
By default an input's validity is checked via `checkValidity()` on blur, and the styles are updated
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this Textfield.valid out to another PR

@@ -324,6 +324,10 @@ with the corresponding id within the document and automatically assign it to thi
Boolean. Proxies to the foundation's `isDisabled/setDisabled` methods when retrieved/set
respectively.

##### MDCTextfield.valid
Copy link
Contributor

Choose a reason for hiding this comment

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

Move out to Textfield.valid PR

@@ -57,6 +57,7 @@ export default class MDCTextfieldFoundation extends MDCFoundation {
this.inputBlurHandler_ = () => this.deactivateFocus_();
this.inputInputHandler_ = () => this.autoCompleteFocus_();
this.inputKeydownHandler_ = () => this.receivedUserInput_ = true;
this.useNativeValidityChecking_ = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this entire file out to Textfield.valid PR

@lynnmercier lynnmercier self-assigned this Aug 7, 2017
@teoxoy teoxoy closed this Aug 7, 2017
@teoxoy
Copy link
Author

teoxoy commented Aug 7, 2017

I split the 3 issues across 3 branches in my fork and i closed this one.

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.

3 participants