Skip to content

fix(menu): Set menu-item icon color only when not set on mat-icon - #8614

Merged
tinayuangao merged 6 commits into
angular:masterfrom
glenstaes:patch-1
Dec 1, 2017
Merged

fix(menu): Set menu-item icon color only when not set on mat-icon#8614
tinayuangao merged 6 commits into
angular:masterfrom
glenstaes:patch-1

Conversation

@glenstaes

Copy link
Copy Markdown
Contributor

Set the theme icon color only if the color input property was not set on the mat-icon component.

Fixes#8594

Set the theme icon color only if the `color` input property was not set on the `mat-icon` component.
Fixes#8594
@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 23, 2017
Add unit tests for setting the mat-icon[color] attribute on a menu item
@glenstaesglenstaes reopened this Nov 23, 2017
@glenstaesglenstaes changed the title fix(menu): Set menu-item icon color if not set on mat-iconfix(menu): Set menu-item icon color only when not set on mat-iconNov 23, 2017
Comment threadsrc/lib/menu/menu.spec.ts Outdated
expect(fixture.componentInstance.items.first.getLabel()).toBe('Item');
});

it('should have an icon with the default theme color if mat-icon has no color attribute set',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can get rid of these tests since they're basically testing the CSS engine.

Comment threadsrc/lib/menu/menu.spec.ts Outdated
dir = 'ltr';
TestBed.configureTestingModule({
imports: [MatMenuModule, NoopAnimationsModule],
imports: [MatIconModule, MatMenuModule, NoopAnimationsModule],

Copy link
Copy Markdown
Member

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 having dependencies between components like this. It shouldn't be an issue after the previous comment about removing the tests is addressed.

@glenstaes

Copy link
Copy Markdown
ContributorAuthor

@crisbeto Comments have been addressed

@crisbetocrisbeto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@crisbetocrisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 29, 2017
@tinayuangao
tinayuangao merged commit d0cb077 into angular:masterDec 1, 2017
@angular-automatic-lock-bot

Copy link
Copy Markdown

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-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 7, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<mat-icon> ignores color attribute if inside <button mat-menu-item>

5 participants

@glenstaes@crisbeto@googlebot@josephperrott@tinayuangao