Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

fix(list): Update default notifyAction impl to emit object #4356

Merged
merged 6 commits into from
Feb 5, 2019

Conversation

acdvorak
Copy link
Contributor

@acdvorak acdvorak commented Feb 5, 2019

Fixes #4355

BREAKING CHANGE: The default MDCListAdapter.notifyAction() implementation now emits an object of type {index: number} rather than a primitive number directly.

Fixes #4355

BREAKING CHANGE: The default `MDCListAdapter.notifyAction()` implementation now emits an object of type `{index: number}` rather than a primitive `number` directly.
@codecov-io
Copy link

codecov-io commented Feb 5, 2019

Codecov Report

Merging #4356 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4356   +/-   ##
=======================================
  Coverage   98.56%   98.56%           
=======================================
  Files         130      130           
  Lines        5731     5731           
  Branches      763      763           
=======================================
  Hits         5649     5649           
  Misses         82       82
Impacted Files Coverage Δ
packages/mdc-list/index.js 98.47% <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 701ed5c...6152bf8. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 621 screenshot tests passed for commit d99a2b3 vs. master! 💯🎉

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.

Do we not have documentation about the format of this event in the README right now? That might actually help for this change, but we should also probably add it... (See e.g. Dialog's Events section)

@mdc-web-bot
Copy link
Collaborator

All 621 screenshot tests passed for commit 9dea6a5 vs. master! 💯🎉

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.

Also update the notifyAction test in mdc-list.test.js? (It seems we aren't strenuously testing the event detail, but we're also passing a number to it.)

@mdc-web-bot
Copy link
Collaborator

All 621 screenshot tests passed for commit 9a29392 vs. master! 💯🎉

@acdvorak
Copy link
Contributor Author

acdvorak commented Feb 5, 2019

Updated unit test to assert.deepEqual(event.detail, {index: 3}).

@mdc-web-bot
Copy link
Collaborator

All 621 screenshot tests passed for commit 6152bf8 vs. master! 💯🎉

@acdvorak acdvorak merged commit ed1aeb2 into master Feb 5, 2019
@acdvorak acdvorak deleted the fix/list/notifyAction-api branch February 5, 2019 22:08
acdvorak added a commit that referenced this pull request Feb 7, 2019
This fixes a regression caused by PR #4356, and adds a unit test to catch future integration failures between list and menu events.
acdvorak added a commit that referenced this pull request Feb 7, 2019
This fixes a regression caused by PR #4356, and adds a unit test to catch future integration failures between list and menu events.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants