Breaking: Radio and RadioGroup components #723

lightbringer1991 · 2018-05-29T06:48:24Z

Creates a new Radio and RadioGroup components
Consolidates prop types between Radio and Checkbox; RadioGroup and CheckboxGroup

TODO:

tests
IE11 compatibility check
uninstall react-icheck (may need to go as a separate PR since it affects Checkbox and CheckboxGroup usage as well)

Description

Motivation and Background Context

Resolves #700

Does this PR introduce a breaking change?

Yes
No
Radio buttons API has been changed from react-icheck API to adslot-ui (see documentation)
onClick is no longer supported, onChange should be used instead
data-name is no longer supported, please use dts instead

How Has This Been Tested?

Screenshots (if appropriate):

Check-list:

I have read the Contributing document.
I've thought about and labelled my PR/commit message appropriately.
If this PR introduces breaking changes I've described the impact and migration path for existing applications.
CI is green (coverage, linting, tests).
I have updated the documentation accordingly.
I've two LGTMs/Approvals.
I've fixed or replied to all my code-review comments.
I've manually tested with a buddy.
I've squashed my commits into one.

mdotwills

Looks good

nimishjha · 2018-05-29T06:57:46Z

docs/examples/RadioExample.jsx

+        {
+          propType: 'className',
+          type: 'string',
+          note: 'This class will go into the input element',


will be applied to

nimishjha · 2018-05-29T07:09:06Z

Will the radio component ever be used by itself, i.e. not within a radioGroup? If not, maybe add a check for that and throw an error if someone tries to do so.

lightbringer1991 · 2018-05-29T07:19:08Z

we actually export both Radio and RadioGroup from react-icheck atm, I would prefer to do the same to avoid having it as a breaking change.

codecov-io · 2018-05-30T00:04:37Z

Codecov Report

Merging #723 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #723   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          56     59    +3     
  Lines         862    905   +43     
  Branches      181    185    +4     
=====================================
+ Hits          862    905   +43

Impacted Files	Coverage Δ
src/components/adslot-ui/CheckboxGroup/index.jsx	`100% <100%> (ø)`	⬆️
src/components/prop-types/inputPropTypes.js	`100% <100%> (ø)`
src/components/adslot-ui/Radio/index.jsx	`100% <100%> (ø)`
src/components/adslot-ui/RadioGroup/index.jsx	`100% <100%> (ø)`
src/components/adslot-ui/Checkbox/index.jsx	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8e9650...b0df36a. Read the comment docs.

mdotwills · 2018-05-30T00:21:02Z

src/components/adslot-ui/Radio/index.jsx

+      value,
+    };
+
+    const optionalProps = _.pickBy({


is it possible to grab these out without the pickBy. I was reading somewhere that it is expensive.

xiaofan2406 · 2018-05-30T00:22:57Z

src/components/adslot-ui/Radio/index.jsx

+
+    return (
+      <div className="radio-component" {...expandDts(dts)}>
+        <input {...radioInputProps} {...optionalProps} />


not sure why we do this. I feel it is much clear to just lay it out as jsx, and only spread the optional cuz spread by default removes the undefined.

ChaoLiangSuper · 2018-05-30T00:49:03Z

Had to pull your branch and ran it locally.
I dont think the style is the same as compared to the previous. Can you check with UX team? Or @omgaz

Used to be:

New style:

nimishjha

LGTM, apart from the appearance issue raised by @ChaoLiangSuper

lightbringer1991 · 2018-05-31T01:58:26Z

just investigated symphony code, this change and the Checkbox change will break symphony quite a bit, since the syntax of onChange function changes from (event, value) => {...} to (event) => {...}

nimishjha · 2018-05-31T02:25:52Z

What's even worse is that value there is actually the checked state, i.e. a boolean. The reason react-icheck works like that is explained here. We need to investigate whether we'll end up skinning the checkbox using SVGs and custom divs, in which case we might have the same problem, i.e. needing to pass through the checked state as an argument. Perhaps for now the best solution is to make the changes necessary to be backward-compatible with current use cases. It'd be annoying if we get Symphony to change their code for this, then have to ask them to change it back later when (if) we skin the checkboxes/radio buttons.

omgaz · 2018-05-31T03:52:29Z

Echo @ChaoLiangSuper's design comment. The padding and spacing is off and needs to be resolved. Make sure the label is 5px from the input and 20px (10px each) between them. This should also be the case for the checkboxes.

lightbringer1991 · 2018-05-31T04:20:09Z

@omgaz sounds good, I'll make it happen

regarding the onChange function signature, do you think we can move on with having it like this

onChange = (event) => { ... }

or like this

onChange = (event, checked) => { ... }

omgaz · 2018-06-03T23:56:23Z

src/components/adslot-ui/Radio/index.jsx

+RadioButton.defaultProps = {
+  disabled: false,
+  checked: false,
+};


I'm wondering if you think it might be worthwhile to share the prop types between radios and checkboxes to ensure they have a shared API. I think it will help ensure consistency between the two components that make sense separated as components.

interesting, I'll investigate that possiblity.

Or as an improvement, maybe we can export only one component. And let the user passing a parameter to choose whether they want a checkbox or radio. Then we can share all other props.
🌚Note: breaking changes v26, LOL🌝

I think we should still export them separately, as per icheck.
Combining checkbox and radio into 1 component is a bit over complicated imo.

It makes sense to keep them separate, I think they get grouped together too often because they feel similar. I think sharing a prop type API is a good way of having consistency without coupling their logic and forcing branches in the code to differentiate logic.

ChaoLiangSuper · 2018-06-04T00:14:47Z

src/components/adslot-ui/Radio/styles.scss

+    padding-left: 5px;
+  }
+
+  padding-top: 10px;


Combine L12 and L13 together? padding: 10px 0 ?

I think it's easier to read this way so I separate them out.

ChaoLiangSuper

Before the merge, please make sure you have already rebased the latest master and used the svg for display. When you ready, I'm happy to do a cross-platform testing for you.
Otherwise, looking good to me.

lteacher · 2018-06-05T00:05:51Z

src/components/adslot-ui/Radio/index.jsx

+      id,
+      className,
+      'data-name': this.props['data-name'],
+    };


Is this the new standard for setting props on the input? I usually like to just see them on the input so wondering why we need the extra var and then later spread?

this was done on Checkbox to accommodate symphony's usage of that particular data-* attribute. I think I need to discuss with Gary about this, and potentially remove data-* props and let symphony changes their code.

also it looks cleaner to me as well

lteacher · 2018-06-05T00:06:31Z

src/components/adslot-ui/Radio/index.jsx

+
+  onChangeDefault(event) {
+    this.setState({ checked: Boolean(event.target.checked) });
+    if (this.props.onChange) {


Why don't we just use defaultProps with _.noop to set these functions? then won't have to keep doing these conditional checks.

One problem with using defaultProps like that here is that if you change the props during the lifetime of the component and don't specify onChange when you do so, it gets reset to to the defaultProps value, i.e. _.noop.

Im not sure i understand how that works, if it changes with no onChange shouldn't it reset? what else should it be

Well, let's say you create a radioButton with an initial set of props, including an onChange function. Later on, you want to change the props, say, to set the radiobutton to disabled without changing anything else. I think it's better to be able to do that by just passing in a disabled prop without having to also pass in all the other props we passed in earlier.

most of the time we actually just modify the props object and pass all of them back into the component, so this is generally not a problem. You can set it as _.noop.

Maybe it will help reducing the branches while testing as well.

Im still struggling to get that implementation, is there somewhere to refer to that has an example? Sounds like some dynamic props definition which is passed to the component like:

const initialProps = { checked: false, disabled: false, onChange: doAmazingStuff, } getRadioProps = (props = initialProps) => props; // Somewhere out there in the wild when change happened <Radio {...getRadioProps({ disabled: true })} />

Which doesn't sound like the right implementation to me?

I think its the right move to pass all the props back from a consumer perspective. There might be some custom implementation such as defaults described, or shouldComponentUpdate check which depends on the props, or even the getDerivedStateFromProps so generally i would figure its not the best practice to do anything like my example unless sure its what is desired, i.e a totally new component with totally new props not just some kind of update.

Well, yeah, we could just pass in all the props every time, I really don't prefer one way over another.

lteacher · 2018-06-05T00:09:19Z

src/components/adslot-ui/RadioGroup/index.jsx

+
+  render() {
+    const { dts, className, id } = this.props;
+    const componentProps = _.pickBy({


Whats the pickBy for? isnt this just the object given since I dont see a predicate.

ChaoLiangSuper

Local tested on both chrome and IE 11.

LGTM

ChaoLiangSuper · 2018-06-06T01:56:06Z

Merging....🌚

mdotwills reviewed May 29, 2018

View reviewed changes

nimishjha reviewed May 29, 2018

View reviewed changes

lightbringer1991 force-pushed the 700-add-radio-button-component branch 2 times, most recently from 3681ed9 to ef78c7a Compare May 29, 2018 23:58

mdotwills reviewed May 30, 2018

View reviewed changes

xiaofan2406 reviewed May 30, 2018

View reviewed changes

lightbringer1991 force-pushed the 700-add-radio-button-component branch 2 times, most recently from 8b4dfef to 3af2337 Compare May 30, 2018 01:14

nimishjha approved these changes May 31, 2018

View reviewed changes

lightbringer1991 force-pushed the 700-add-radio-button-component branch 2 times, most recently from 8284f43 to 29b7b00 Compare June 1, 2018 01:36

omgaz reviewed Jun 3, 2018

View reviewed changes

ChaoLiangSuper reviewed Jun 4, 2018

View reviewed changes

lightbringer1991 force-pushed the 700-add-radio-button-component branch from 29b7b00 to 1f0f8ed Compare June 4, 2018 07:07

lteacher reviewed Jun 5, 2018

View reviewed changes

lightbringer1991 force-pushed the 700-add-radio-button-component branch 2 times, most recently from bace57b to 59caa27 Compare June 5, 2018 01:00

omgaz approved these changes Jun 5, 2018

View reviewed changes

lteacher approved these changes Jun 5, 2018

View reviewed changes

lightbringer1991 force-pushed the 700-add-radio-button-component branch from 59caa27 to 8459d00 Compare June 5, 2018 04:58

ChaoLiangSuper approved these changes Jun 5, 2018

View reviewed changes

lightbringer1991 force-pushed the 700-add-radio-button-component branch from f1c0e26 to f10167d Compare June 5, 2018 06:00

lightbringer1991 changed the title ~~New: Radio and RadioGroup components~~ Breaking: Radio and RadioGroup components Jun 5, 2018

lightbringer1991 force-pushed the 700-add-radio-button-component branch from f10167d to c8fcbc0 Compare June 5, 2018 06:10

Breaking: Radio and RadioGroup components

b0df36a

lightbringer1991 force-pushed the 700-add-radio-button-component branch from c8fcbc0 to b0df36a Compare June 5, 2018 06:42

ChaoLiangSuper merged commit 44553ae into master Jun 6, 2018

ChaoLiangSuper deleted the 700-add-radio-button-component branch June 6, 2018 01:56

Breaking: Radio and RadioGroup components #723

Breaking: Radio and RadioGroup components #723

Conversation

lightbringer1991 commented May 29, 2018 • edited Loading

Description

Motivation and Background Context

Does this PR introduce a breaking change?

How Has This Been Tested?

Screenshots (if appropriate):

Check-list:

mdotwills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimishjha commented May 29, 2018

lightbringer1991 commented May 29, 2018

codecov-io commented May 30, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaoLiangSuper commented May 30, 2018

nimishjha left a comment

Choose a reason for hiding this comment

lightbringer1991 commented May 31, 2018

nimishjha commented May 31, 2018

omgaz commented May 31, 2018

lightbringer1991 commented May 31, 2018

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

ChaoLiangSuper 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

lightbringer1991 Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

lteacher Jun 5, 2018 • edited Loading

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

ChaoLiangSuper left a comment

Choose a reason for hiding this comment

ChaoLiangSuper commented Jun 6, 2018

lightbringer1991 commented May 29, 2018 •

edited

Loading

codecov-io commented May 30, 2018 •

edited

Loading

lightbringer1991 Jun 5, 2018 •

edited

Loading

lteacher Jun 5, 2018 •

edited

Loading