Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(md-chips): change md-icon class to inherit size #10491

Merged
merged 2 commits into from
Jun 30, 2020
Merged

fix(md-chips): change md-icon class to inherit size #10491

merged 2 commits into from
Jun 30, 2020

Conversation

batsauto
Copy link
Contributor

@batsauto batsauto commented Mar 13, 2017

fix(chips): md-chips remove icon not inheriting class size

This fix will allow the chip remove icon to inherit the proper size from the css class.

This addresses issue #9619

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Mar 13, 2017
@batsauto batsauto changed the title possible fix for md-chips remove icon being larger in 1.1.1 (#9619) fix(md-chips) remove icon being larger in 1.1.1 (#9619) Mar 14, 2017
@batsauto batsauto changed the title fix(md-chips) remove icon being larger in 1.1.1 (#9619) fix(md-chips) remove icon being larger in 1.1.1 Mar 14, 2017
@topherfangio topherfangio self-requested a review March 20, 2017 21:25
@topherfangio topherfangio self-assigned this Mar 20, 2017
@topherfangio topherfangio added this to the 1.1.4 milestone Mar 20, 2017
@topherfangio
Copy link
Contributor

Looks good, but can you please update the commit message & PR description to follow our guidelines?

https://github.com/angular/material/blob/master/.github/CONTRIBUTING.md#submitting-pull-requests

Thanks!

Copy link
Contributor

@topherfangio topherfangio left a comment

Choose a reason for hiding this comment

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

Please see above.

@batsauto batsauto changed the title fix(md-chips) remove icon being larger in 1.1.1 fix(md-chips) remove icon too large Mar 20, 2017
@batsauto batsauto changed the title fix(md-chips) remove icon too large fix(md-chips) change md-icon class to inherit size Mar 20, 2017
@batsauto batsauto changed the title fix(md-chips) change md-icon class to inherit size fix(md-chips): change md-icon class to inherit size Mar 20, 2017
This will allow the chip remove icon to inherit the proper size from the css class
change your code in src/components/chips/chips.scss from
md-icon {
height: $chip-delete-icon-size;
width: $chip-delete-icon-size;
position: absolute;
top: 50%;
left: 50%;
transform: translate3d(-50%, -50%, 0);
}
to this
md-icon {
height: $chip-delete-icon-size;
width: $chip-delete-icon-size;
min-height: $chip-delete-icon-size;
min-width: $chip-delete-icon-size;
position: absolute;
top: 50%;
left: 50%;
transform: translate3d(-50%, -50%, 0);
}

fixes issue #9619
@batsauto
Copy link
Contributor Author

I updated the commit message and PR description

@topherfangio
Copy link
Contributor

@batsauto Based on what I'm seeing in your changes, this should not be classified as a breaking commit because the user shouldn't need to change anything. Also, your description in the PR still says that you need to change chips.scss, but you've already done that.

Am I not understanding something?

Thanks!

@batsauto
Copy link
Contributor Author

batsauto commented Mar 21, 2017

I accidently placed the commit message info into the PR description. It has been fixed. Thanks for your understanding.

@topherfangio
Copy link
Contributor

LGTM 👍

@ThomasBurleson ThomasBurleson modified the milestones: 1.1.5, 1.1.6 Sep 5, 2017
@Splaktar Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Nov 21, 2017
@Splaktar Splaktar added type: bug P1: urgent Urgent issues that should be addressed in the next minor or patch release. labels Dec 4, 2017
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM. Verified on docs site.

@Splaktar Splaktar modified the milestones: 1.1.6, 1.1.7 Jan 17, 2018
@Splaktar Splaktar self-assigned this Jan 22, 2018
@Splaktar Splaktar modified the milestones: 1.1.7, Future Jan 22, 2018
@Splaktar
Copy link
Member

This would normally have to go into a minor release and not a patch release since there are CSS changes. But I need to review how we should handle this for the case of patch release regressions.

By the time a minor release comes out, we'll hopefully be aligning with the Material Design spec as called out in #9883.

@Splaktar
Copy link
Member

This breaking CSS change was part of the 1.1.0 minor release as the CSS is changed from what was in 1.0.9. As this was a minor release, breaking CSS changes like this were acceptable. Additionally, we'll need to wait for another minor release (1.2.0) before this can be fixed since it is a breaking visual change via CSS.

@Splaktar Splaktar modified the milestones: 1.1.7, Future Jan 22, 2018
@Splaktar Splaktar modified the milestones: Future, 1.2.0 Jan 29, 2018
@Splaktar
Copy link
Member

Feedback for presubmit failures: This PR "has mostly good looking screenshot diffs, except one app where the chip remove button isn't vertically centered." It's about 4px lower than it should be.

@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: manual testing This issue or PR needs to have some manual testing and verification done and removed pr: merge ready This PR is ready for a caretaker to review labels Aug 30, 2018
@Splaktar Splaktar added pr: lgtm This PR has been approved by the reviewer and removed pr: presubmit-failures in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: manual testing This issue or PR needs to have some manual testing and verification done labels Apr 22, 2020
@Splaktar Splaktar requested a review from mmalerba June 29, 2020 23:37
@Splaktar Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Jun 29, 2020
@Splaktar Splaktar removed the request for review from mmalerba June 30, 2020 17:51
@Splaktar Splaktar merged commit 29c0a4a into angular:master Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P1: urgent Urgent issues that should be addressed in the next minor or patch release. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review severity: regression This issue is related to a regression type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants