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(checkbox): remove native control from getters/setters of foundation #3408

Merged
merged 10 commits into from
Aug 28, 2018

Conversation

moog16
Copy link
Contributor

@moog16 moog16 commented Aug 23, 2018

refs #3396

Will move the ObjectPropertyDescriptor override into the component in a follow up PR

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit e3c7c9d vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit ecd0520 vs. master! 💯🎉

@codecov-io
Copy link

codecov-io commented Aug 23, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@52a9eda). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #3408   +/-   ##
========================================
  Coverage          ?   98.3%           
========================================
  Files             ?     119           
  Lines             ?    5010           
  Branches          ?     613           
========================================
  Hits              ?    4925           
  Misses            ?      85           
  Partials          ?       0
Impacted Files Coverage Δ
packages/mdc-checkbox/index.js 100% <100%> (ø)
packages/mdc-checkbox/foundation.js 97.22% <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 52a9eda...4ac34a7. Read the comment docs.

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.

Are you intending for the issue to remain open until you do the follow-up PR? If so, change "fixes" to "refs" in the PR description (otherwise it's going to close it when merged)

@@ -292,8 +260,8 @@ class MDCCheckboxFoundation extends MDCFoundation {
}

updateAriaChecked_() {
// Ensure aria-checked is set to mixed if checkbox is in indeterminate state.
if (this.isIndeterminate()) {
// Ensure aria-checked is set to if checkbox is in indeterminate state.
Copy link
Contributor

Choose a reason for hiding this comment

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

You accidentally deleted mixed somehow

@moog16
Copy link
Contributor Author

moog16 commented Aug 24, 2018

@kfranqueiro I don't want to do the changes until this PR is merged in, since it is highly dependent on this PR.

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit b99202f vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 349 screenshot tests passed for commit ad97f84 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 349 screenshot tests passed for commit 1ee6735 vs. master! 💯🎉

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.

Readme needs updating.

* @private
*/
isIndeterminate_() {
return this.nativeCb_.indeterminate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move these to the get methods and then update the adapter to use the public get ? Seems like we don't really need the private function.

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

@mdc-web-bot
Copy link
Collaborator

All 349 screenshot tests passed for commit 0f7fecc vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 349 screenshot tests passed for commit 4ac34a7 vs. master! 💯🎉

@moog16 moog16 merged commit b0fe9cf into master Aug 28, 2018
@moog16 moog16 deleted the fix/checkbox/remove-native-control branch August 28, 2018 00:04
@jamesmfriedman jamesmfriedman mentioned this pull request Sep 26, 2018
49 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants