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(icon-button): update event handling to new standard #3165

Merged
merged 38 commits into from
Jul 26, 2018

Conversation

moog16
Copy link
Contributor

@moog16 moog16 commented Jul 21, 2018

fixes #3162

@@ -61,18 +61,10 @@ class MDCIconButtonToggleFoundation extends MDCFoundation {

This comment was marked as resolved.

const {foundation, mockAdapter} = setupTest();
foundation.init();
foundation.handleClick();
td.verify(mockAdapter.setAttr('aria-pressed', 'true'), {times: 1});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should verify that foundation.toggle() was called. The aria-pressed is verified in another test.

@bonniezhou bonniezhou self-assigned this Jul 23, 2018
@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 3e9b8d0 vs. master:

No diffs! 💯🎉

Copy link
Contributor

@bonniezhou bonniezhou left a comment

Choose a reason for hiding this comment

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

Also remove registerInteractionHandler/deregisterInteractionHandler from adapter.js. Otherwise LGTM!

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 6cc8985 vs. master:

No diffs! 💯🎉

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit c36f23a vs. master:

No diffs! 💯🎉

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 12ed874 vs. master:

No diffs! 💯🎉

Matt Goo added 2 commits July 25, 2018 10:42
…aterial-components/material-components-web into fix/icon-button/event-handler-standard
@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit ad47552 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.

[de]registerInteractionHandler are still in the readme and should be removed

td.verify(component.foundation_.handleClick(), {times: 1});
});

test('keydown handler is removed from the root element on destroy', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

keydown -> click

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 6f13d36 vs. master:

No diffs! 💯🎉

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit 4facd12 vs. master:

No diffs! 💯🎉

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report

Commit c1c1d38 vs. master:

No diffs! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit c2e55bf vs. master! 💯🎉

Matt Goo added 2 commits July 25, 2018 16:01
…aterial-components/material-components-web into fix/icon-button/event-handler-standard
@codecov-io
Copy link

codecov-io commented Jul 25, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3165      +/-   ##
==========================================
- Coverage   98.12%   98.12%   -0.01%     
==========================================
  Files         101      101              
  Lines        4426     4421       -5     
  Branches      585      585              
==========================================
- Hits         4343     4338       -5     
  Misses         83       83
Impacted Files Coverage Δ
packages/mdc-icon-button/index.js 100% <100%> (ø) ⬆️
packages/mdc-icon-button/foundation.js 95.74% <100%> (-0.56%) ⬇️

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 547a980...9079594. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit 6e8fd5d vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit f937f13 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit a395dd6 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit a1ff5ac vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit 44d35e7 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit db77f72 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit 9079594 vs. master! 💯🎉

@moog16 moog16 merged commit 531867e into master Jul 26, 2018
@moog16 moog16 deleted the fix/icon-button/event-handler-standard branch July 26, 2018 22: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 Icon Button toggle event to match new event handler pattern
5 participants