Skip to content

fix(checkbox): not handling unknown color palette #18467

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.

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

Merged
merged 1 commit into from
Aug 21, 2020

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