Skip to content

Conversation

@wagnermaciel
Copy link
Contributor

…nentHarness

  • Allows users to access mat-icons within mat-buttons

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 24, 2020
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM, but needs an update to the API goldens.

@wagnermaciel wagnermaciel requested a review from a team as a code owner September 24, 2020 18:10
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM, just the one comment

Comment on lines +126 to +131
<button id="home-icon" type="button" mat-icon-button>
<mat-icon>home</mat-icon>
</button>
<button id="favorite-icon" type="button" mat-icon-button>
<mat-icon>favorite</mat-icon>
</button>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't one of these test cases remain as a mat-icon-button without a mat-icon element inside?

Suggested change
<button id="home-icon" type="button" mat-icon-button>
<mat-icon>home</mat-icon>
</button>
<button id="favorite-icon" type="button" mat-icon-button>
<mat-icon>favorite</mat-icon>
</button>
<button id="home-icon" type="button" mat-icon-button>
<mat-icon>home</mat-icon>
</button>
<button id="favorite-icon" type="button" mat-icon-button>favorite</button>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to have two mat-icon-buttons with mat-icons inside so that we could make sure that the correct mat-icon was being returned.

Also, the mat-icon with text in it was being used for any tests. @mmalerba What are your thoughts here?

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 the icon button with text in it was just a sloppy previously written test. I'm good with these 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

@mmalerba mmalerba added action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround labels Sep 24, 2020
@mmalerba
Copy link
Contributor

Note for a followup: once #20646 goes in we might as well tighten it up so we're looking under the element where the content is projected

@wagnermaciel wagnermaciel force-pushed the content-container-button-harness branch from 8e10729 to 4695702 Compare September 29, 2020 12:31
…nentHarness

* Allows users to access mat-icons within mat-buttons
@wagnermaciel wagnermaciel force-pushed the content-container-button-harness branch from 4695702 to 4f38868 Compare October 2, 2020 14:01
@mmalerba mmalerba added the target: minor This PR is targeted for the next minor release label Oct 2, 2020
@mmalerba mmalerba merged commit 8cacf64 into angular:master Oct 2, 2020
@angular-automatic-lock-bot
Copy link

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 Nov 2, 2020
@wagnermaciel wagnermaciel deleted the content-container-button-harness branch January 14, 2021 19:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants