Skip to content

Conversation

@crisbeto
Copy link
Member

Fixes the checkbox component not falling back to the default color, if the color is undefined. We need to handle this, because ThemePalette allows undefined.

Fixes #18465.

@crisbeto crisbeto 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 Feb 11, 2020
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 11, 2020
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Feb 11, 2020
@crisbeto crisbeto force-pushed the 18465/checkbox-no-color branch 2 times, most recently from 0410756 to ca251a7 Compare February 23, 2020 18:14
@alfaproject
Copy link

Any hope to get this merged? Just spent hours trying to figure out if the issue was my custom styles and then stumbled upon this...

@jelbourn jelbourn 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 Jul 16, 2020
@jelbourn
Copy link
Member

Bumping to P2 because changing the default click action without setting the color leads to a broken checkbox

@jelbourn
Copy link
Member

@crisbeto this has a very subtle breakage. If someone uses the provider to set the default color to primary and also has a color input that sets the value to undefined with the intent to use the default color, this now makes the color accent instead of primary. I think for the case when the value is undefined, we should fall back to the default provided into the config rather than always accent.

@crisbeto
Copy link
Member Author

I never considered that @jelbourn, but it sounds like this is broken inside the mixinColor which is responsible for assigning the default when the current color is undefined. I'll submit a separate PR to fix it and the tabindex mixins, because it could break the apps of people that depend on the current behavior.

crisbeto added a commit to crisbeto/material2 that referenced this pull request Jul 29, 2020
This is something that came up in angular#18467. Currently the default values in the `mixinColor` and `mixinTabIndex` are set when the mixin class is created, however we have some components where the default color can be configured through an injection token. These changes add a default field on each instance so that it can be updated after the class is instantiated.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Jul 29, 2020
This is something that came up in angular#18467. Currently the default values in the `mixinColor` and `mixinTabIndex` are set when the mixin class is created, however we have some components where the default color can be configured through an injection token. These changes add a default field on each instance so that it can be updated after the class is instantiated.
@crisbeto crisbeto added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Jul 29, 2020
@crisbeto
Copy link
Member Author

Blocked on #20125.

@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@andrewseguin
Copy link
Contributor

One more change will be needed - the MDC checkbox doesn't have a get color checking for the defaultColor #20125, which means setting the color after construction will cause the component to forget about falling back to defaultColor.

Either a get color should be added, or we can check in ngOnChanges if color is falsy and set it back to the options color.

andrewseguin pushed a commit that referenced this pull request Aug 13, 2020
#20125)

This is something that came up in #18467. Currently the default values in the `mixinColor` and `mixinTabIndex` are set when the mixin class is created, however we have some components where the default color can be configured through an injection token. These changes add a default field on each instance so that it can be updated after the class is instantiated.
@andrewseguin andrewseguin removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 14, 2020
Fixes the checkbox component not falling back to the default color, if the color is `undefined`. We need to handle this, because `ThemePalette` allows `undefined`.

Fixes angular#18465.
@crisbeto crisbeto force-pushed the 18465/checkbox-no-color branch from ca251a7 to aa9ceeb Compare August 16, 2020 08:36
@crisbeto
Copy link
Member Author

Updated to add the same behavior to the MDC checkbox.

@wagnermaciel wagnermaciel merged commit 64145fd into angular:master Aug 21, 2020
wagnermaciel pushed a commit that referenced this pull request Aug 21, 2020
Fixes the checkbox component not falling back to the default color, if the color is `undefined`. We need to handle this, because `ThemePalette` allows `undefined`.

Fixes #18465.

(cherry picked from commit 64145fd)
@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 Sep 21, 2020
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: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Angular 9][mat-checkbox] undefined color input doesn't fallback to the default color

8 participants