Skip to content

fix(material-experimental/mdc-button): use mdc mixins to style icons within buttons #21623

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 19, 2021
Merged

fix(material-experimental/mdc-button): use mdc mixins to style icons within buttons #21623

merged 2 commits into from
Feb 19, 2021

Conversation

@annieyw
Copy link
Contributor

@annieyw annieyw commented Jan 19, 2021 •
edited
Loading

Add appropriate class to mat-icon when it is within a MDC button to receive the correct styles.
(This is technically a fix for material-experimental/mdc-button but the change is in material/icon)
Before
Screen Shot 2021-01-19 at 7 01 45 PM
After
Screen Shot 2021-01-19 at 7 02 10 PM

@annieyw annieyw added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Jan 19, 2021
@annieyw annieyw requested a review from jelbourn January 19, 2021 11:06
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 19, 2021
@annieyw annieyw force-pushed the mdc-button-icon-class branch from 96793aa to 55e566c Compare January 19, 2021 11:08
crisbeto
crisbeto reviewed Jan 19, 2021
View reviewed changes
src/material/icon/icon.ts Outdated

// Add appropriate MDC class if it is inside a MDC button to be styled properly
if (this._elementRef.nativeElement.parentElement?.classList.contains('mdc-button')) {
this._elementRef.nativeElement.classList.add('mdc-button__icon');
Copy link
Member

@crisbeto crisbeto Jan 19, 2021

Choose a reason for hiding this comment

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

In general, we try to avoid dependencies between components, especially between MDC and non-MDC versions. I see a couple of alternatives:

  1. Redeclare the styles for .mdc-button__icon, but have it target .mat-mdc-button .mat-icon instead. I'm guessing that MDC has a mixin for it already.
  2. Have the MDC-based button look for icons instead. This is a bit fragile, because the icon can come in at a later point.

Copy link
Contributor

@mmalerba mmalerba Jan 19, 2021

Choose a reason for hiding this comment

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

I think this PR had fixed it at one point: #20963 did something change that broke it again?

@annieyw annieyw requested a review from mmalerba January 19, 2021 16:51
@annieyw annieyw force-pushed the mdc-button-icon-class branch from 55e566c to 13bf892 Compare January 21, 2021 09:49
@annieyw annieyw requested a review from andrewseguin as a code owner January 21, 2021 09:49
@annieyw
Copy link
Contributor Author

annieyw commented Jan 21, 2021

The material-components-web package needs to be updated before this will work (checked locally with a newer version)

@annieyw annieyw changed the title fix(material/icon): style icons properly when within a MDC button fix(material-experimental/mdc-button): use mdc mixins to style icons within buttons Jan 21, 2021
mmalerba
mmalerba approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM, approving but marking as blocked until the MDC update goes in, feel free to remove the label once it goes in

@mmalerba mmalerba added blocked This issue is blocked by some external factor, such as a prerequisite PR action: merge The PR is ready for merge by the caretaker labels Jan 21, 2021
@crisbeto
Copy link
Member

crisbeto commented Jan 21, 2021

The version was updated in acb3f33.

@mmalerba mmalerba removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Jan 21, 2021
@mmalerba
Copy link
Contributor

mmalerba commented Jan 21, 2021

cool, unblocking then

@andrewseguin
Copy link
Contributor

andrewseguin commented Jan 25, 2021

This breaks a fair amount of screenshots but I wonder if we can easily provide an override that reverses the size. Then in the clients breaking, we just bring the size back to what they are expecting

@mmalerba
Copy link
Contributor

mmalerba commented Jan 26, 2021

@annieyw it looks like this only fixes text buttons, the other types of button still appear to have an icon that's too big

@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Jan 26, 2021
@annieyw annieyw force-pushed the mdc-button-icon-class branch from 13bf892 to e822d14 Compare January 26, 2021 15:34
@annieyw
Copy link
Contributor Author

annieyw commented Jan 26, 2021

Turns out the rest of the buttons was missing the button.icon() mixin that changes the size

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Jan 26, 2021
@mmalerba mmalerba added P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Feb 4, 2021
@andrewseguin andrewseguin merged commit a96fbc6 into angular:master Feb 19, 2021
andrewseguin pushed a commit that referenced this pull request Feb 19, 2021
@annieyw @andrewseguin
fix(material-experimental/mdc-button): use mdc mixins to style icons …
61deb71 
…within buttons (#21623)

* fix(material-experimental/mdc-button): use mdc mixins to style icons within buttons

* fix(material-experimental/mdc-button): fix icon size for unelevated, raised, and outlined

(cherry picked from commit a96fbc6)
@annieyw annieyw deleted the mdc-button-icon-class branch February 19, 2021 21:06
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Mar 22, 2021

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto left review comments

@mmalerba mmalerba mmalerba approved these changes

@jelbourn jelbourn Awaiting requested review from jelbourn

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

+1 more reviewer

@andreLagacione andreLagacione andreLagacione approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@annieyw @crisbeto @mmalerba @andrewseguin @andreLagacione