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(select): reduce adapter apis not used in MDCReact and update events to new pattern #3204

Merged
merged 12 commits into from
Jul 25, 2018

Conversation

moog16
Copy link
Contributor

@moog16 moog16 commented Jul 25, 2018

fixes #3198

@moog16 moog16 requested a review from kfranqueiro July 25, 2018 01:30
@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 1bc6d77 vs. master:

No diffs! 💯🎉

@codecov-io
Copy link

codecov-io commented Jul 25, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3204      +/-   ##
==========================================
- Coverage   98.13%   98.12%   -0.01%     
==========================================
  Files         101      101              
  Lines        4444     4426      -18     
  Branches      585      585              
==========================================
- Hits         4361     4343      -18     
  Misses         83       83
Impacted Files Coverage Δ
packages/mdc-select/index.js 90.1% <100%> (+0.69%) ⬆️
packages/mdc-select/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 8286045...793e0f4. 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.

It occurred to me this morning that maybe we should also rename setDisabled on the foundation, since it's not actually doing the setting itself anymore. Maybe updateDisabledStyle or something?

Also, this will need some BREAKING CHANGE notes when we merge it, e.g.: Removed some adapter APIs and shifted responsibility of event handling and programmatic select element updates out of the foundation and into the component.

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 35dee08 vs. master:

No diffs! 💯🎉

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 4cf3ffd vs. master:

No diffs! 💯🎉

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 2004983 vs. master:

No diffs! 💯🎉

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.

Don't forget to add BREAKING CHANGE: ... to the commit description when merging (see my previous comment)

@moog16
Copy link
Contributor Author

moog16 commented Jul 25, 2018

Will do!

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 793e0f4 vs. master:

No diffs! 💯🎉

@moog16 moog16 merged commit e29742a into master Jul 25, 2018
@moog16 moog16 deleted the experimental/select-fewer-adapter-apis branch July 25, 2018 21:06
@jamesmfriedman jamesmfriedman mentioned this pull request Jul 30, 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.

Update Select Eventing by removing registerInteractionHandler and deregisterInteractionHandler adapter methods
5 participants