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

feat(switch): Move component specific logic out of foundation #3342

Merged
merged 8 commits into from
Aug 15, 2018

Conversation

lynnmercier
Copy link
Contributor

No description provided.

@mdc-web-bot
Copy link
Collaborator

All 337 screenshot tests passed for commit 14f81ee vs. master! 💯🎉

@codecov-io
Copy link

codecov-io commented Aug 14, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3342      +/-   ##
==========================================
- Coverage   98.13%   98.12%   -0.01%     
==========================================
  Files         123      123              
  Lines        5194     5185       -9     
  Branches      638      638              
==========================================
- Hits         5097     5088       -9     
  Misses         97       97
Impacted Files Coverage Δ
packages/mdc-switch/index.js 97.95% <100%> (-0.05%) ⬇️
packages/mdc-switch/foundation.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 dd20ea8...9c0081b. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 337 screenshot tests passed for commit 528bc95 vs. master! 💯🎉

@kfranqueiro kfranqueiro self-requested a review August 14, 2018 13:59
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.

Removing the init code from the foundation regresses #3147. (The two #init tests you removed are meaningful to this.)

Maybe we can relocate that handleChange call to the component's initialSyncWithDOM, but we would also need to pass in a mock event/target based on the initial state, and that seems maybe a little hacky.

@lynnmercier
Copy link
Contributor Author

Re: #3147, "Firefox would remember the state of the hidden input element on refresh "

How do I reproduce this bug?

@kfranqueiro
Copy link
Contributor

In Firefox:

Or, in Chrome, it can be reproduced via browser history:

  • Toggle the first switch
  • Use the arrow in the MDC Toolbar (not the browser's back button) to navigate to the index page
  • Use the browser's back button to go back to the switch page
  • Toggle the first switch again

During the last step, the switch doesn't move... because it was actually already toggled, but its UI wasn't in sync with the fact that the browser recalled its state.

@lynnmercier
Copy link
Contributor Author

Okay i fixed this bug where the checked state can be out of sync with the styling. I moved the logic to the vanilla component in the initialSyncWithDom method

@mdc-web-bot
Copy link
Collaborator

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

@kfranqueiro kfranqueiro changed the title chore(switch): Move component specific logic out of foundation feat(switch): Move component specific logic out of foundation Aug 15, 2018
@kfranqueiro
Copy link
Contributor

kfranqueiro commented Aug 15, 2018

FYI I think this needs a BREAKING CHANGE: when merged because it removes 2 foundation APIs and makes handleChange require an event parameter.

@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@lynnmercier lynnmercier merged commit e1e4465 into master Aug 15, 2018
@lynnmercier lynnmercier deleted the chore/switch-api branch August 15, 2018 17:53
@jamesmfriedman jamesmfriedman mentioned this pull request Aug 23, 2018
48 tasks
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.

None yet

4 participants