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] Fix regression when disabled #1467

Merged
merged 2 commits into from May 14, 2019
Merged

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented May 14, 2019

WHY are these changes introduced?

It’s unfortunate that we missed this in the build-consumer test. This seems to be a regression caused by not fully understanding #1363. In #1369 I made it so Checkbox cannot be disabled and checked. however, it seems we wanted to remove the ability to fire click events while disabled and keep the ability to change Checkbox checked state.

WHAT is this pull request doing?

  • removing unnecessary code
  • moving logic from Choice to Checkbox
  • updating tests

How to 🎩

  • Simple playground to toggle checked and disabled states ⬇️&
  • Build-consumer and check the failing test in web
Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {Checkbox, Button} from '@shopify/polaris';

interface State {
  checked: boolean;
  disabled: boolean;
}

export default class Playground extends React.Component {
  state = {
    checked: true,
    disabled: false,
  };

  render() {
    const {checked, disabled} = this.state;

    return (
      <div>
        <Checkbox
          checked={checked}
          disabled={disabled}
          label="Basic checkbox"
          onChange={this.handleChange}
        />
        <Button
          onClick={() => this.setState(({disabled}) => ({disabled: !disabled}))}
        >
          toggle disabled
        </Button>
        <Button
          onClick={() => this.setState(({checked}) => ({checked: !checked}))}
        >
          toggle checked
        </Button>
      </div>
    );
  }

  handleChange = (value) => {
    this.setState({checked: value});
  };
}

@@ -45,27 +45,10 @@ const getUniqueID = createUniqueIDFactory('Checkbox');
class Checkbox extends React.PureComponent<CombinedProps, never> {
private inputNode = React.createRef<HTMLInputElement>();

componentDidMount() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the logic that caused the regression -> componentDidMount/Update

@@ -165,6 +158,18 @@ describe('<Checkbox />', () => {
);
expect(element.find('input').prop('disabled')).toBeFalsy();
});

it('can change values when disabled', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this test to prevent regression in the future

@@ -35,20 +35,14 @@ export default function Choice({
helpText,
onClick,
}: Props) {
function handleClick() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like this logic is out of scope for the Choice component, so it was moved to the handleInput function in Checkbox

@AndrewMusgrave
Copy link
Member Author

@danrosenthal If this looks good to you and the logic makes sense / 🎩 / review are ✅ , feel free to merge this so you can release a patch

Copy link
Contributor

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

Changes and code coverage look great.

This fixes the test failure in web and my 🎩 confirms it also fixes the associated UI regression.

@danrosenthal danrosenthal merged commit 9e21e95 into master May 14, 2019
@danrosenthal danrosenthal deleted the fix-checkbox-regression branch May 14, 2019 14:36
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.

None yet

3 participants