Skip to content
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

[Checkbox] Small fix in checkbox logic #1453

Merged
merged 1 commit into from
May 10, 2019
Merged

[Checkbox] Small fix in checkbox logic #1453

merged 1 commit into from
May 10, 2019

Conversation

keyfer
Copy link
Contributor

@keyfer keyfer commented May 10, 2019

WHY are these changes introduced?

Saw this code while running through some component logic, thought it was worth the quick fix.

WHAT is this pull request doing?

Simple fix to checkbox logic.

@keyfer keyfer self-assigned this May 10, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-1453 May 10, 2019 13:47 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1453 May 10, 2019 13:49 Inactive
danrosenthal
danrosenthal previously approved these changes May 10, 2019
this.props.disabled &&
!prevDisabled !== true &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes my eyes go crossed. Good fix

Copy link
Contributor Author

@keyfer keyfer May 10, 2019

Choose a reason for hiding this comment

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

Obviously could just make this prevDisabled &&, but I'd rather avoid Type Coercion.

@danrosenthal danrosenthal dismissed their stale review May 10, 2019 14:13

Looks like it's actually breaking in CI...

@danrosenthal danrosenthal self-requested a review May 10, 2019 14:13
@BPScott BPScott temporarily deployed to polaris-react-pr-1453 May 10, 2019 17:34 Inactive
@keyfer keyfer merged commit cd868a1 into master May 10, 2019
@keyfer keyfer deleted the checkbox/small-tweak branch May 10, 2019 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants