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(chips): implements missing adapter method notifyTrailingIconInteraction #653

Conversation

mgr34
Copy link
Contributor

@mgr34 mgr34 commented Jan 31, 2019

Implements missing adapter method notifyTrailingIconInteraction. Along with introducing shouldRemoveOnTrailingIconClick as ChipProp. (mdc-web may consider renaming this to shouldRemoveOnTrailingIconInteraction?)

A successful merge would close #652 and #271. Additionally would cause a breaking change where removeIcon is renamed to trailingIcon.

Screenshot tests were updated, however, golden.json may need attention.


I signed it

@codecov-io
Copy link

codecov-io commented Jan 31, 2019

Codecov Report

Merging #653 into rc0.10.0 will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           rc0.10.0    #653      +/-   ##
===========================================
+ Coverage     94.89%   94.9%   +0.01%     
===========================================
  Files            68      68              
  Lines          2838    2847       +9     
  Branches        426     428       +2     
===========================================
+ Hits           2693    2702       +9     
  Misses           50      50              
  Partials         95      95
Impacted Files Coverage Δ
packages/chips/Chip.tsx 97.97% <100%> (+0.2%) ⬆️

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 9841e18...c7a842a. Read the comment docs.

@mgr34 mgr34 force-pushed the fix/chips/notifyTrailingIconInteraction branch from 5bca8a1 to 951ee49 Compare January 31, 2019 21:57
@mgr34 mgr34 changed the title fix(chips): implements missing foundation method notifyTrailingIconInteraction fix(chips): implements missing adapter method notifyTrailingIconInteraction Feb 2, 2019
packages/chips/Chip.tsx Outdated Show resolved Hide resolved
Copy link

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

This is awesome! Thanks so much for fixing this. There are a lot of missing pieces to chips so I'm really excited you put so much work into this.

please update the golden to:

f5973cc5f1961464cbbbe152ca25b9a989e7e5a54b6d64cb28f0c25788f7df44

@moog16
Copy link

moog16 commented Feb 6, 2019

@mgr34 looks like you have a conflict with readme.md

@mgr34
Copy link
Contributor Author

mgr34 commented Feb 6, 2019

@moog16 -- not just that it also looks like I accidentally committed my desire to update all dates to 2019...ugh. I thought I stashed all that.

Hold on, this needs some work.

@mgr34
Copy link
Contributor Author

mgr34 commented Feb 6, 2019

@moog16 -- merge resolve and commit reverted. Thought I had stashed my attempt to update everything to 2019.

Copy link

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

looks good!

@moog16 moog16 merged commit 585aced into material-components:rc0.10.0 Feb 6, 2019
@moog16
Copy link

moog16 commented Feb 6, 2019

This is a breaking change...I will ammend the commit history. removeIcon --> trailingIcon

moog16 pushed a commit that referenced this pull request Feb 6, 2019
…raction (#653)

BREAKING CHANGE: renamed chip.props.removeIcon --> chips.props.trailingIcon
@mgr34
Copy link
Contributor Author

mgr34 commented Feb 6, 2019

to be clear I meant for this to be a breaking change and that removeIcon should now be trailingIcon. Semantically this is the logical choice.

@moog16
Copy link

moog16 commented Feb 6, 2019

yup that was my fault, thank you!

moog16 pushed a commit that referenced this pull request Feb 19, 2019
…raction (#653)

BREAKING CHANGE: renamed chip.props.removeIcon --> chips.props.trailingIcon
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.

3 participants