Move `Toggle`, `Radio`, and `Checkbox` to inputs #428

montezume · 2019-01-11T14:03:14Z

Summary

Moves Checkbox, Radio, and Switch out of the src/components/switches folder into the src/components/inputs folder. Also moves them to the inputs section of Storybook.

BREAKING CHANGE

Toggle's now returns an event from it's onChange with the shape

{
      target: {
        id: this.props.id,
        name: this.props.name,
        checked: !this.props.isChecked,
      },
}

Toggle is now ToggleInput
Radio is now RadioInput
Checkbox is now CheckboxInput

This is to unify the API with all the other inputs (including Checkbox and Radio), which already emit events.

Closes #318

montezume · 2019-01-11T14:09:32Z

src/components/inputs/toggle/toggle.js

@@ -22,6 +22,17 @@ export class Toggle extends React.PureComponent {
    size: 'big',
  };

+  emitChange = () => {
+    const event = {
+      target: {


this is the breaking change

montezume · 2019-01-11T14:09:38Z

src/components/inputs/time-input/time-input.spec.js

@@ -72,7 +72,7 @@ describe('TimeInput', () => {
    expect(container.querySelector('input')).toHaveAttribute('value', 'foo');
  });

-  it('should call onChange when chaning the value', () => {
+  it('should call onChange when changing the value', () => {


tdeekens

Thanks! Super nice to get them to have the same API! Hare we sure that the Toggle is an input. I think so but at the same time wonder...

emmenko

Thanks for looking into this!

emmenko · 2019-01-11T14:22:38Z

src/components/inputs/toggle/toggle.js

+        value: !this.props.isChecked,
+      },
+    };
+    this.props.onChange(event);


Why are we constructing the event ourselves? 🤔

I can't get it to work with the native event. You are welcome to try it, but I spent about an hour on it and got nowhere with RTL. The shape is the same now as the native event though.

Oh really? Sorry about that...

Hmm that's weird, I would have expected this to work. I can have a look over the weekend.

switching to defaultChecked fixed the probelm

emmenko · 2019-01-11T14:24:21Z

src/index.js

-
+export { default as Toggle } from './components/inputs/toggle';
+export { default as Checkbox } from './components/inputs/checkbox';
+export { default as Radio } from './components/inputs/radio';


I think we should name them consistently with the other component, so with Input suffix.

Very valid!

I renamed Toggle -> ToggleInput. If we are happy with this, I will rename the rest of them on Monday

montezume · 2019-01-11T14:33:36Z

It's being used with formik with this workaround in the MC

// FIXME: the Toggle component should send the `event` in the `onChange` handler
function patchToggleChange(isChecked) {
  // This is bound to the function
  const props = this;
  const event = {
    target: {
      type: 'checkbox',
      name: `${props.fieldName}.isActive`,
      checked: isChecked,
    },
  };
  props.formikProps.handleChange(event);
}

emmenko · 2019-01-11T14:38:25Z

It's being used with formik with this workaround in the MC

I know, I wrote that 😄

That's why I'm asking: why not simply onChange={this.props.onChange}?

emmenko · 2019-01-11T14:39:54Z

The input sends the correct event, the workaround was to make it work with formik because the component didn't return an event.

montezume · 2019-01-11T15:29:26Z

I can't get it to work with the native event with RTL.

montezume · 2019-01-11T15:33:17Z

In fact, if I try it in storybook, what is returned from the event, doesn't contain what we need (at least in the console log...)

There's no checked attribute

tdeekens

Should we rename them to be radio-input, checkbox-input and toggle-input?

emmenko · 2019-01-11T16:47:40Z

src/components/inputs/toggle-input/README.md

+| `isDisabled` | `bool`   |    -     | -                       | `false` | Disables the ToggleInput               |
+| `isChecked`  | `bool`   |    -     | -                       | `false` | Checks the ToggleInput                 |
+| `onChange`   | `func`   |    ✅    | -                       | -       | called with the new value.             |
+| `size`       | `string` |    ✅    | oneOf(['small', 'big']) | `big`   | The size of the ToggleInput component. |


Some props are missing here

emmenko · 2019-01-11T16:47:59Z

src/components/inputs/toggle-input/toggle-input.js

@@ -38,7 +38,7 @@ export class Toggle extends React.PureComponent {
          className={styles.inputWrapper}
          id={this.props.id}
          name={this.props.name}
-          onChange={() => this.props.onChange(!this.props.isChecked)}


montezume · 2019-01-14T16:03:34Z

I updated the PR with the renaming of the rest of the switches

emmenko

🎉🙌

montezume added 💥 Type: Breaking Change Semver: BREAKING labels Jan 11, 2019

montezume requested review from dferber90, emmenko and tdeekens and removed request for dferber90 January 11, 2019 14:07

montezume changed the title ~~chore: move Toggle, radio and checkbox to input, emit event for toggle~~ Move Toggle, Radio, and Checkbox to inputs Jan 11, 2019

montezume commented Jan 11, 2019

View reviewed changes

montezume force-pushed the ml-toggle branch from f3a6a50 to 69ae9a4 Compare January 11, 2019 14:13

tdeekens reviewed Jan 11, 2019

View reviewed changes

emmenko reviewed Jan 11, 2019

View reviewed changes

tdeekens reviewed Jan 11, 2019

View reviewed changes

emmenko reviewed Jan 11, 2019

View reviewed changes

montezume force-pushed the ml-toggle branch from 9cc3c83 to c84373c Compare January 14, 2019 14:33

montezume self-assigned this Jan 14, 2019

montezume requested a review from tdeekens January 14, 2019 16:03

tdeekens added the 👨‍👩‍👦‍👦 Status: Dev Review label Jan 14, 2019

tdeekens approved these changes Jan 14, 2019

View reviewed changes

emmenko approved these changes Jan 14, 2019

View reviewed changes

montezume added 3 commits January 16, 2019 09:24

chore: move toggle to inputs and update onChange to mit event

956c2dc

chore(checkbox): move to inputs

ffe12d0

chore(radio): move to inputs

d9f1bbc

montezume added 10 commits January 16, 2019 09:24

chore: update evt

dc0e617

chore: use native event

90b5b97

chore: rename toggle -> toggleinput

3028362

chore: rename toggle

6c9e6d9

chore: fix test

cb34616

chore: fixup readme

68f39de

chore: fixup toggle input readme

83e34c4

chore(checkbox): rename to checkbox-input

737eb58

chore(radio): rename to radio-input

64645e1

chore: fixup readmes

058d454

montezume force-pushed the ml-toggle branch from 23da04a to 058d454 Compare January 16, 2019 08:24

montezume merged commit 4f73e03 into master Jan 16, 2019

montezume deleted the ml-toggle branch January 16, 2019 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `Toggle`, `Radio`, and `Checkbox` to inputs #428

Move `Toggle`, `Radio`, and `Checkbox` to inputs #428

montezume commented Jan 11, 2019 •

edited

Loading

montezume Jan 11, 2019

montezume Jan 11, 2019

tdeekens left a comment

emmenko left a comment

emmenko Jan 11, 2019

montezume Jan 11, 2019

emmenko Jan 11, 2019

montezume Jan 11, 2019

emmenko Jan 11, 2019

tdeekens Jan 11, 2019

montezume Jan 11, 2019

montezume commented Jan 11, 2019

emmenko commented Jan 11, 2019 •

edited

Loading

emmenko commented Jan 11, 2019 •

edited

Loading

montezume commented Jan 11, 2019

montezume commented Jan 11, 2019 •

edited

Loading

tdeekens left a comment

emmenko Jan 11, 2019

emmenko Jan 11, 2019

montezume commented Jan 14, 2019

emmenko left a comment

Move Toggle, Radio, and Checkbox to inputs #428

Move Toggle, Radio, and Checkbox to inputs #428

Conversation

montezume commented Jan 11, 2019 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdeekens left a comment

Choose a reason for hiding this comment

emmenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume commented Jan 11, 2019

emmenko commented Jan 11, 2019 • edited Loading

emmenko commented Jan 11, 2019 • edited Loading

montezume commented Jan 11, 2019

montezume commented Jan 11, 2019 • edited Loading

tdeekens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume commented Jan 14, 2019

emmenko left a comment

Choose a reason for hiding this comment

Move `Toggle`, `Radio`, and `Checkbox` to inputs #428

Move `Toggle`, `Radio`, and `Checkbox` to inputs #428

montezume commented Jan 11, 2019 •

edited

Loading

emmenko commented Jan 11, 2019 •

edited

Loading

emmenko commented Jan 11, 2019 •

edited

Loading

montezume commented Jan 11, 2019 •

edited

Loading