Skip to content

fix(material/checkbox): no color assigned if defaults don't have a color - #21042

Merged
wagnermaciel merged 1 commit into
angular:masterfrom
crisbeto:21031/checkbox-defaults-color
Nov 17, 2020
Merged

fix(material/checkbox): no color assigned if defaults don't have a color#21042
wagnermaciel merged 1 commit into
angular:masterfrom
crisbeto:21031/checkbox-defaults-color

Conversation

@crisbeto

@crisbeto crisbeto commented Nov 13, 2020

Copy link
Copy Markdown
Member

Fixes an issue where a mat-checkbox wouldn't have a color, if the MAT_CHECKBOX_DEFAULT_OPTIONS was provided without one.

Fixes #21031.

@crisbeto crisbeto added 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 labels Nov 13, 2020
@google-cla google-cla Bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 13, 2020
crisbeto
crisbeto commented Nov 13, 2020
View reviewed changes
Comment thread src/material-experimental/mdc-checkbox/checkbox.ts
let nextUniqueId = 0;

// Default checkbox configuration.
const defaults = MAT_CHECKBOX_DEFAULT_OPTIONS_FACTORY();

@crisbeto crisbeto Nov 13, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Invoking the factory directly feels a little weird, but I'm trying to avoid repeating the default color across different files.

@mmalerba mmalerba Nov 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I'm not even sure why its a factory, feels like it could just be provided with useValue. I'm fine with doing this, we can revisit if it ever becomes problematic.

@crisbeto
fix(material/checkbox): no color assigned if defaults don't have a color
cb9875b 
Fixes an issue where a `mat-checkbox` wouldn't have a `color`, if the
`MAT_CHECKBOX_DEFAULT_OPTIONS` was provided without one.

Fixes angular#21031.
@crisbeto
crisbeto force-pushed the 21031/checkbox-defaults-color branch from 74e0134 to cb9875b Compare November 13, 2020 16:52
mmalerba
mmalerba approved these changes Nov 16, 2020
View reviewed changes

@mmalerba mmalerba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread src/material-experimental/mdc-checkbox/checkbox.ts
let nextUniqueId = 0;

// Default checkbox configuration.
const defaults = MAT_CHECKBOX_DEFAULT_OPTIONS_FACTORY();

@mmalerba mmalerba Nov 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I'm not even sure why its a factory, feels like it could just be provided with useValue. I'm fine with doing this, we can revisit if it ever becomes problematic.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Nov 16, 2020
@wagnermaciel
wagnermaciel merged commit 5e9cc5f into angular:master Nov 17, 2020
wagnermaciel pushed a commit that referenced this pull request Nov 17, 2020
@crisbeto @wagnermaciel
fix(material/checkbox): no color assigned if defaults don't have a co…
c1a65c3 
…lor (#21042)

Fixes an issue where a `mat-checkbox` wouldn't have a `color`, if the
`MAT_CHECKBOX_DEFAULT_OPTIONS` was provided without one.

Fixes #21031.

(cherry picked from commit 5e9cc5f)
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Dec 18, 2020

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-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@devversion devversion Awaiting requested review from devversion
@jelbourn jelbourn Awaiting requested review from jelbourn
1 more reviewer
@mmalerba mmalerba mmalerba approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mmalerba mmalerba

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.

MatCheckbox not rendering check for undefined color when using default options

3 participants

@crisbeto @mmalerba @wagnermaciel