[ADS-1046] Checkbox/CheckboxGroup components #716

nimishjha · 2018-05-02T23:50:56Z

Description

Motivation and Background Context

#700
https://adslot.atlassian.net/browse/ADS-1046

Does this PR introduce a breaking change?

Yes
No

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.

nimishjha · 2018-05-07T01:04:16Z

Will change CheckboxSingle to CheckBox when it's good to go. Also, please ignore the console logs for now.

omgaz · 2018-05-07T05:36:49Z

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

+      name: props.name,
+      value: props.value,
+      dts: props.dts,
+      label: props.label,


state, dts, and name don't need to be managed by state they should remain in the props.

omgaz · 2018-05-07T05:37:44Z

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

+		this.state = {
+      checked: props.checked,
+      disabled: props.disabled,
+      name: props.name,


Do we need name, if we have value/label. Can we derive this?

We need it for usage in a group, because in that case name will be shared across all child checkboxes.

omgaz · 2018-05-07T05:39:38Z

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

+        name={this.state.name}
+        value={this.state.value}
+        onChange={onChangeHandler}
+        data-test-selector={this.state.dts}


We have an expandDts helper we can use for this.

https://github.com/Adslot/adslot-ui/blob/master/src/lib/utils.js

omgaz · 2018-05-07T05:41:39Z

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

@@ -0,0 +1,9 @@
+.checkbox-single label {
+	line-height: 20px;


This seems too much, I think it might be 16px - let me check.

Yup, can you make this 16px to match the w/h of the checkbox

omgaz · 2018-05-07T05:52:23Z

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

+}
+
+.checkbox-single input {
+	margin: 2px 5px 0 0;


As a label can be optional, we should only add a margin on the right if there's one present. Maybe wrap the label text in a span and add a margin-left to that span?

omgaz · 2018-05-07T05:53:23Z

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

+};
+
+CheckboxSingle.defaultProps = {
+  label: '',


A label is optional and should be allowed to be undefined and shouldn't default to ''.

omgaz · 2018-05-07T05:54:28Z

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

+
+CheckboxSingle.propTypes = {
+  name: PropTypes.string,
+  label: PropTypes.string,


We should allow for PropTypes.node so we can pass through formatted html e.g. <span>Required <HelpIconPopover>This field is required</HelpIconPopover></span>

omgaz · 2018-05-07T05:56:27Z

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

+	constructor(props) {
+		super(props);
+		this.state = {
+      checked: props.checked,


Just some small indentation commit issues.

omgaz · 2018-05-07T06:00:52Z

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

+      value: this.props.value || [],
+    };
+    this.renderChildren = this.renderChildren.bind(this);
+    this.onChangeCallback = this.onChangeCallback.bind(this);


The arrow function binding issue for Adslot UI should have been merged, I think we can adopt the new style now:

https://adslot.atlassian.net/wiki/spaces/APT/pages/178225251/React

Can you have a look, if it fails to work we should re-open the issue.

it should be working. We already have some code that adapt this style, I think.

It's not, I get a "Parsing error: unexpected token ="

^ did you run npm i?

Yes. I think @omgaz mentioned yesterday that the arrow function update had been applied to direct-web, but not adslot-ui.

Definitely the plugin isnt there. I added randomly in some PR. Would be nice to have though in another PR maybe I should do that.

omgaz

I think we could make use of a shouldComponentUpdate check on the value/checked/disabled in the single checkbox to get some perf gains on large lists where we have a checkbox (ie the grid)

omgaz · 2018-05-07T06:03:50Z

Getting there! Good effort with your first React Component.

mdotwills · 2018-05-07T23:41:10Z

docs/examples/CheckboxGroupExample.jsx

+import React from 'react';
+import Example from '../components/Example';
+import { CheckboxSingle } from '../../src';
+import { CheckboxGroup } from '../../src';


this can be just import { CheckboxSingle, CheckboxGroup } from '../../src';

Yep, fixing.

mdotwills · 2018-05-07T23:42:01Z

docs/examples/CheckboxGroupExample.jsx

+import { CheckboxSingle } from '../../src';
+import { CheckboxGroup } from '../../src';
+
+const Konsole = window.console;


is this a special or wrapped console.log?

Just an alias, to get around the linter. Only for development purposes, will be gone soon.

mdotwills · 2018-05-07T23:46:16Z

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

+    this.onChangeCallback = this.onChangeCallback.bind(this);
+  }
+
+  onChangeCallback(checkboxParams, event) {


Maybe for consistency we can drop the Callback in the function name here, we don't do it anywhere else. We can just have it as onChange (see here and specifically here) 👍

There's already an onChange function which is always called, the onChangeCallback is optional and is only called when it's provided. Can rename this to something else, open to suggestions.

Im prob not a fan so much of the extra onChangeCallback as well mostly just because its an extra prop that shouldn't be externally used by the consumer as you just need it for the group. So um I guess my comment is really for the actual Checkbox...

I did have some trouble seeing where the onChange sits on the Checkbox itself though, seems like there is no actual prop there?

For the individual checkbox, there's a default onChange that's always called, to update its state, and if an onChangeCallback has been provided, whether from a parent CheckboxGroup or another component, onChange() calls it after it's finished executing. I'm open to suggestions for the function names, or if someone has a better approach to solve the problem, that's also welcome.

mdotwills · 2018-05-08T00:04:50Z

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

+  onChangeCallback(checkboxParams, event) {
+    let newValue;
+    if(checkboxParams.checked){
+      newValue = this.state.value.concat([checkboxParams.value]);


I see two issues here, the first being we are modifying the state directly here (see here), which is not permissible to do outside of the constructor. The other issue is that we are setting the state based on the previous value of the same state (see here and here). Due to the way React bunches state updates, if we need to know the previous state during setState, we need to use the function call of setState to guarantee we are operating on the latest updated state before setting it:

if(checkboxParams.checked) { this.setState((prevState) => ({ value: prevState.concat([checkboxParams.value] })); } else { this.setState(prevState) => ({ value: prevState.filter(currentValue => currentValue !== checkboxParams.value) })); }

(just double check the logic there)

also notice if you want that you can write the (prevState) => ({ value: prevState.concat([checkboxParams.value] }) part as a function, so

const updater = (prevState) => ({ value: prevState.concat([checkboxParams.value] });

and then later

this.setState(updater);

(obv. better names are needed than this example haha)

concat doesn't mutate the existing arrays - it returns a new array, so state is not being modified there. The state is updated in a setState call on line 28.

good point about concat not mutating state. But the latter point still remains, you need to call setState with a function and not an object.

Actually, no, because I'm not accessing the current value of state in setState at all.

@nimishjha you are, via newValue which depends on this.state.value

Yes, but I'm not accessing current state inside setState...

Went ahead and changed to function syntax for setState anyway. No harm in following best practices...

Does seem like @mdotwills has a valid point though also seems like there are a few components everywhere in direct-web and maybe here too that are not exactly correct or could suffer from the issue.

I mean the documentation doesn't refer to anything being done in the anon function execution of setState, it specifically refers to a .state.

This example from the documentation is using the state in the current scope

this.setState({ // this.state.counter is in the outer scope and being applied to the object passed to setState counter: this.state.counter + this.props.increment, });

mdotwills · 2018-05-08T00:16:32Z

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

+
+    if(label)
+    {
+  		return(


I think the linter might not have picked up the inconsistent indentation here

mdotwills · 2018-05-08T00:26:44Z

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

+      disabled: isDisabled,
+    });
+    if(this.onChangeCallback) {
+      this.onChangeCallback({value: this.state.value, checked: isChecked}, event, this.props.name);


If you needed to guarantee that this.state.value was completely up-to-date, you could move this to be inside of the callback after the object argument version of setState.

this.setState({ checked: isChecked, disabled: isDisabled, }, () => { if(this.onChangeCallback) { this.onChangeCallback({ value: this.state.value, checked: isChecked }, event, this.props.name); } });

My understanding is that if there are bunched state updates waiting from other setState calls, this callback will capture the updated state from all updates.
(Note: callback may fire even if the state does not update). But given we're not updating this.state.value here, it should be okay as is. Just something to consider when debugging.

Yeah, I'm aware of that.

mdotwills · 2018-05-08T00:27:02Z

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

+CheckboxGroup.propTypes = {
+  name: PropTypes.string.isRequired,
+  value: PropTypes.arrayOf(PropTypes.string),
+	children: PropTypes.arrayOf(PropTypes.element).isRequired,


Hmm, no idea where the random tab came from. Fixed.

ChaoLiangSuper

Looking GTM

xiaofan2406 · 2018-05-11T01:41:18Z

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

+
+  render() {
+    const { name, value, label, dts } = this.props;
+    if (label) {


this conditional re-render will cause the input to be unmounted and lost its all native values, because the tree is different

Should render the same structure, with or without the label, and do the conditional check in L57 {label ? <span>{label}</span>} : null

Good to know, thanks.

xiaofan2406 · 2018-05-11T01:47:04Z

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

+  label: PropTypes.node,
+  value: PropTypes.string.isRequired,
+  dts: PropTypes.string,
+  disabled: PropTypes.bool,


the disabled input attribute is fixed with this.state.disabled, then the only change it can happen is in onChangeDefault which reads from input attribute.

Meaning that, you can remove disabled state completely, and just use disabled prop

Not sure what you mean...

xiaofan2406 · 2018-05-11T01:49:01Z

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

+  constructor(props) {
+    super(props);
+    this.state = {
+      value: this.props.value || [],


use default props. and remove the || here

Actually, have to keep it this way, or the defaultProps override the props when you pass them in with value undefined.

Can you leave that in a comment so someone doesn't unintentionally refactor this out.

omgaz

I'll carry on reviewing after the meeting. Can discuss changes if needed.

omgaz · 2018-05-14T04:28:51Z

docs/examples/CheckboxGroupExample.jsx

+        <CheckboxSingle label="Predator" value="predator" />
+        <CheckboxSingle label="The Sound of Music" value="soundofmusic" />
+      </CheckboxGroup>
+  `,


Kinda good kinda bad way in which back-ticks work across multi-line. They're great because they allow multi-line strings but they also mean we get string literal line breaks for the start and end (as well as spaces).

Can we update so that it'll render correctly as:

exampleCodeSnippet: `<CheckboxGroup name="movies" value={['terminator', 'predator']} onChange={onChangeGroup}> <CheckboxSingle label="The Terminator" value="terminator" onChange={onChangeIndividual} /> <CheckboxSingle label="Predator" value="predator" /> <CheckboxSingle label="The Sound of Music" value="soundofmusic" /> </CheckboxGroup>`

so the html rendering looks like:

<CheckboxGroup name="movies" value={['terminator', 'predator']} onChange={onChangeGroup}> <CheckboxSingle label="The Terminator" value="terminator" onChange={onChangeIndividual} /> <CheckboxSingle label="Predator" value="predator" /> <CheckboxSingle label="The Sound of Music" value="soundofmusic" /> </CheckboxGroup>

omgaz · 2018-05-14T04:32:07Z

docs/examples/CheckboxGroupExample.jsx

+        },
+        {
+          propType: 'value',
+          type: 'Array of strings',


It looks like we use the arrayOf terminology. e.g. arrayOf(string) although this doesn't seem to be consistent.

omgaz · 2018-05-14T04:32:55Z

docs/examples/CheckboxSingleExample.jsx

+      disabled={true}
+      dts="data-test-selector-goes-here"
+    />
+  `,


ditto with the example snippet indentation.

omgaz · 2018-05-14T04:34:34Z

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

+  constructor(props) {
+    super(props);
+    this.state = {
+      value: this.props.value || [],


Can you leave that in a comment so someone doesn't unintentionally refactor this out.

omgaz · 2018-05-14T04:41:00Z

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

+    this.onChangeDefault = this.onChangeDefault.bind(this);
+  }
+
+  onChangeDefault(checkboxParams, event) {


Why do we need checkboxParams here, doesn't the event/prevState/state give us everything we need? We have onChangeDefault in the single checkbox only accept event which seems more straightforward.

True, removed checkboxParams and now only using event.

omgaz · 2018-05-14T04:54:45Z

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

+    super(props);
+    this.state = {
+      value: this.props.value || [],
+    };


I disagree and your setState below proves array.indexOf isn't usable in the solution (i.e having to filter by value rather than knowing the index and having an if/else block). Tuan's suggestion of an associative array creates a consistent way of handling the data through look-up rather than having to rely later on to do a filter based on value.

We'd need a getter to allow us to return as an array, and have the constructor be a little bit smarter at setting the internal state from the provided value but it'd really clean up the setState function below.

omgaz · 2018-05-14T04:55:13Z

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

+      if (checkboxParams.checked) {
+        newValue = prevState.value.concat([checkboxParams.value]);
+      } else {
+        newValue = prevState.value.filter(currentValue => currentValue !== checkboxParams.value);


as we can't rely on array.indexOf an associative array with lookup would greatly help

omgaz · 2018-05-14T04:55:34Z

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

+    let newValue;
+
+    this.setState(prevState => {
+      if (checkboxParams.checked) {


shouldn't need this at all if we can just toggle the existing value. Also, I think this creates a disparity, why do we need checkbox params AND prevState, surely prevState should be our source of truth - if it's not reliable as such there's probably a bigger issue somewhere.

But this is the checkboxGroup -- its state is ['value1','value2'...], not like a single checkbox.

omgaz · 2018-05-14T04:57:58Z

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

+        newValue = prevState.value.filter(currentValue => currentValue !== checkboxParams.value);
+      }
+      if (this.props.onChange) {
+        this.props.onChange(newValue, event, this.props.name);


Can this onChange introduce side effects? What happens if this fails? I think, because this is passed from the user we shouldn't put potentially unsafe (or asyncronous stuff) in the state transformation but move it to the callback of setState.

@lightbringer1991 @xiaofan2406 thoughts?

omgaz · 2018-05-14T05:00:01Z

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

+  render() {
+    const { name, value, label, dts } = this.props;
+    return (
+      <div className="checkbox-single">


Is this div necessary? Can we just not export the root as label?

One problem with removing the divs is that labels are inline by default, so the checkboxes/labels display in a row instead of in a column. Setting display: block on the labels solves that problem, but introduces a new one, namely that clicking on the whitespace beyond the text still toggles the checkbox, which doesn't seem desirable.

omgaz · 2018-05-15T06:37:36Z

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

+      this.state.value[child.props.value] = false;
+    });
+    if (this.props.value) {
+      this.state.value = mapPropsToState(this.props, this.state.value);


We should probably call getDerivedStateFromProps?

Wouldn't it get called automatically when the props change? It's a lifecycle method, I'm using it in place of the deprecated componentWillReceiveProps.

constructor() is only called once when the component initializes, while getDerivedStateFromProps() is called every time props or state changes.

I think this block should be in getDerivedStateFromProps(), but I haven't tested it out yet. I assume that getDerivedStateFromProps() is called when component initializes.

Yes, it's called when the component is instantiated, as well as when props change. Why do you think that block of code needs to be in getDerivedStateFromProps()? Do we envision the child checkboxes changing during the life of a CheckboxGroup?

@nimishjha I think we need the state to be updated whenever props are changed, so it can be controlled from outside via props. With the current implementation, we can only pass in the props on initialization, and once it's done, there is no control over the values of the checkboxes inside CheckboxGroup.

I might be missing something here, this is just my thoughts when reading the PR. I may need to pull your branch to investigate further, just to play around with the scenarios.

That's exactly what's happening in getDerivedStateFromProps() here. Check the tests, I have a couple of them that test that scenario.

ahh yep, I missed the function before constructor(), so I guess we don't need this block here then, as it will be handled in getDerivedStateFromProps() anyway.

We do need the block here, because this is where the state object gets initialized - we loop over the children checkboxes, get each child's value and attach a property of that name to the state object. Then if props have a value array, we map that to the state object. getDerivedStateFromProps handles subsequent props changes.

nimishjha · 2018-05-17T04:26:57Z

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

+Checkbox.propTypes = {
+  id: PropTypes.string,
+  className: PropTypes.string,
+  'data-name': PropTypes.string,


data-name is for backwards compatibility with Symphony.

lightbringer1991 · 2018-05-17T06:01:29Z

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

@@ -0,0 +1,14 @@
+.checkbox-single {


should it be just .checkbox?

lightbringer1991

LGTM 👍

lightbringer1991 · 2018-05-17T07:01:44Z

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

+  onChange: PropTypes.func,
+};
+
+CheckboxGroup.defaultProps = {};


just nitpicking here, you can remove this line.

Nitpicks are important... fixed.

omgaz · 2018-05-18T00:13:03Z

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

@@ -0,0 +1,14 @@
+.checkbox {


As a global CSS class I can see this is going to cause us issues when we come to put this into direct. For example, this namespace is used by bootstrap and I believe it's used across our angular/non-angular/react code base poorly.

A reason why grid work as prefixes and why other components here suffix with -component. Not ideal, but I can see problems.

Maybe .checkbox-component

Yeah, that's why I originally had .checkbox-single, but changed it to .checkbox (with misgivings) in response to a previous review.

nimishjha · 2018-05-18T00:37:54Z

Closing this PR as this page keeps crashing. Stay tuned for new PR.

nimishjha force-pushed the nj-icheck-replacement branch 6 times, most recently from 76b4dc4 to ed3dfe5 Compare May 7, 2018 01:00

nimishjha force-pushed the nj-icheck-replacement branch from ed3dfe5 to f0cd907 Compare May 7, 2018 03:25

omgaz reviewed May 7, 2018

View reviewed changes

nimishjha force-pushed the nj-icheck-replacement branch from 61852b4 to 89ab18d Compare May 7, 2018 07:20

mdotwills reviewed May 7, 2018

View reviewed changes

nimishjha force-pushed the nj-icheck-replacement branch from 89ab18d to c2f072d Compare May 7, 2018 23:52

mdotwills reviewed May 8, 2018

View reviewed changes

nimishjha force-pushed the nj-icheck-replacement branch 2 times, most recently from 7435268 to 481fe8e Compare May 8, 2018 01:28

ChaoLiangSuper reviewed May 11, 2018

View reviewed changes

nimishjha force-pushed the nj-icheck-replacement branch 3 times, most recently from 0f5dae4 to eba4a14 Compare May 11, 2018 01:36

xiaofan2406 reviewed May 11, 2018

View reviewed changes

nimishjha force-pushed the nj-icheck-replacement branch from eba4a14 to f405b42 Compare May 11, 2018 02:28

lteacher approved these changes May 14, 2018

View reviewed changes

omgaz suggested changes May 14, 2018

View reviewed changes

nimishjha force-pushed the nj-icheck-replacement branch 8 times, most recently from 585c9c2 to 7956252 Compare May 15, 2018 03:51

omgaz reviewed May 15, 2018

View reviewed changes

nimishjha force-pushed the nj-icheck-replacement branch from 9febfb1 to d1b3ce6 Compare May 17, 2018 04:24

nimishjha commented May 17, 2018

View reviewed changes

nimishjha changed the title ~~[WIP] [ADS-1046] Checkbox/CheckboxGroup components~~ [ADS-1046] Checkbox/CheckboxGroup components May 17, 2018

lightbringer1991 reviewed May 17, 2018

View reviewed changes

nimishjha force-pushed the nj-icheck-replacement branch from 268bbb9 to 3b845d9 Compare May 17, 2018 06:57

lightbringer1991 approved these changes May 17, 2018

View reviewed changes

Update: Checkbox and CheckboxGroup components

f69fb71

nimishjha force-pushed the nj-icheck-replacement branch from 3b845d9 to f69fb71 Compare May 17, 2018 07:07

omgaz reviewed May 18, 2018

View reviewed changes

nimishjha closed this May 18, 2018

[ADS-1046] Checkbox/CheckboxGroup components #716

[ADS-1046] Checkbox/CheckboxGroup components #716

Conversation

nimishjha commented May 2, 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:

nimishjha commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omgaz May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omgaz May 7, 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

Choose a reason for hiding this comment

omgaz May 7, 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

omgaz May 7, 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

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

omgaz left a comment

Choose a reason for hiding this comment

omgaz commented May 7, 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

Choose a reason for hiding this comment

mdotwills May 8, 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

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

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

omgaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimishjha commented May 2, 2018 •

edited

Loading

omgaz May 7, 2018 •

edited

Loading

omgaz May 7, 2018 •

edited

Loading

omgaz May 7, 2018 •

edited

Loading

omgaz May 7, 2018 •

edited

Loading

mdotwills May 8, 2018 •

edited

Loading