init/style Checkbox & CheckboxGroup #29

chamkank · 2018-08-01T05:09:24Z

👷 Changes

This PR adds the Checkbox and CheckboxGroup components.

Usage:

You can use Checkbox on its own but it's cleaner when wrapped with CheckboxGroup because you can have a shared name attribute (as well as shared defaultChecked, disabled, and onChange attributes). You can override a shared prop by passing a specific value for that prop to the child component.

import React from 'react';
import { Checkbox, CheckboxGroup } from '../input/buttons/CheckboxGroup';

<CheckboxGroup name="test_name">
  <Checkbox label="Label 1" value="test_value" checked="checked"/>
  <Checkbox label="Label 2" value="test_value" disabled />
  <Checkbox label="Label 3" value="test_value" name="overriden_name"/>
</CheckboxGroup>

💭 Notes

If you're curious as to what the pure CSS implementation looked like...

Close but not close enough to the mocks, so I used the SVG images instead.

TODO

I still need to add styling for the disabled state
Also need to add Enzyme tests

🔦 Testing Instructions

make web & go to localhost:8080/ui_demo

This lets us only define "name" once (as a prop for CheckboxGroup) and have it be shared by all the child components (Checkboxes).

…defaultChecked)

mingyokim · 2018-08-01T05:23:20Z

web/assets/checkbox/checked.svg

@@ -0,0 +1,4 @@
+<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">


are we pulling svg from a website? is it reliable? why not save it as part of this repo?

xmlns is XML namespacing, which iirc is used to do the svg magic - all svg's will have it

mingyokim · 2018-08-01T05:27:55Z

web/components/input/buttons/CheckboxGroup/index.js

+const InputContext = React.createContext();
+
+const Checkbox = ({ name, value, label, checked, disabled, onChange }) => (
+  <InputContext.Consumer>


This is pretty cool, never seen this pattern before.

Would you still be able to render a Checkbox without CheckboxGroup?

IKR, it's the new context API and I like it a lot. 😃

I intended for Checkbox to be used on its own but currently it's failing without a parent CheckboxGroup. I'll fix this tomorrow, thanks!

mingyokim · 2018-08-01T05:29:04Z

web/components/input/buttons/CheckboxGroup/index.js

+        <input
+          type="checkbox"
+          name={name || context.name}
+          value={value}


Why do we need value? Couldn't onChange just supply the value in the callback?

Not sure if I follow. Normally in regular forms you can have an input without an onChange function (the value is passed on form submit) and so I wanted to support that use case. I haven't played around with forms in React before, is there a reason to have the onChange without the value?

I think this goes for any checkbox form - checkbox is just a toggle, onChange just needs to notify if value is true or false

OH SHOOT, I've mixed up my logic, the shared name prop is only useful to radio buttons (where there is only a single answer), hence why value is there so you can distinguish between radio button options.

But this is a checkbox... 💀 , I'll fix all of this tomorrow.

Actually nevermind, the normal way of handling a group of checkboxes is to group them with the same name but have unique (string) values. Then when you submit the form, all the checked values are passed under the same name and you parse them.

<input type="checkbox" name="diet" value="vegetarian"> Vegetarian <input type="checkbox" name="diet" value="gluten-free" checked> Gluten Free <input type="checkbox" name="diet" value="vegan" checked> Vegan

On form submit this becomes diet=gluten-free&diet=vegan

We can also have unique names for each checkbox and no value (and then parse each one individually by name) but this doesn't feel right if you have a group of checkboxes because you have to prefix them to understand how they're grouped on the front-end (if you don't do this they can conflict with other names in the form which is bad practice).

<input type="checkbox" name="vegetarian"> Vegetarian <input type="checkbox" name="gluten-free" checked> Gluten Free <input type="checkbox" name="vegan" checked> Vegan

On form submit this becomes gluten-free=on&vegan=on

I'm going to stick with the first approach, any objections?

That's fair actually, I think you're right

mingyokim · 2018-08-01T05:29:26Z

web/components/input/buttons/CheckboxGroup/index.js

+  name: PropTypes.string,
+
+  // input value for checkbox
+  value: PropTypes.string.isRequired,


wouldn't this be a boolean?

Which part are you referring to?

checkbox value is either true or false, right? unless value is the label, but it looks like that's covered by name || context.name

chamkank · 2018-08-03T02:36:39Z

Bad news lol, enzyme-adapter-react-16 doesn't support the new context API yet. Stumbled upon it while writing tests (see this error). Good news is that it's being worked on (see this issue). So no tests for this (and the radio button) until that's fixed.

bobheadxi

Looks great!

bobheadxi

jk linter errors:

https://travis-ci.com/nwhacks/nwhacks2019/builds/80827030#L498

bobheadxi · 2018-08-03T04:23:52Z

web/.eslintrc.json

@@ -17,6 +17,7 @@
    "react/jsx-filename-extension": 0,
    "react/require-default-props": 0,
    "react/forbid-prop-types": 0,
+    "react/destructuring-assignment": 0,


Destructuring is awesome - why not enforce it?

I fixed CheckboxGroup to use destructuring (const { children, ...rest } = props;) but that broke another rule which was no-unused-vars (since ...rest wasn't used).

But now I realize that ...rest is optional 😛 , I'll make this change.

bobheadxi

Should be good to go

bobheadxi · 2018-08-03T05:21:21Z

web/.eslintrc.json

+        "customValidators": "customValidator"
+      }
+    ],
+    "jsx-a11y/label-has-for": [ 


should be fine, but do you mind quickly explain what these were added for?

I've removed the prop-types config (it allowed me to not have to write propTypes for CheckboxGroup).

The jsx-a11y/label-has-for config was for dealing with the error related to not having a for attribute for the label. There are two ways I know of to associate labels with checkboxes (which makes the labels clickable):

<input id="some_id"> <label for="some_id">Some label<label>

<label> <input /> Some label </label>

And I chose the second option. With the first one we have to either require that a unique id is passed in to each Checkbox (or we generate one for each Checkbox if it's not provided) which seemed unnecessary. The config I added allows us to use either approach - without the config it forces us to use both, which would look like this:

<label for="some_id"> <input id="some_id" /> Some label </label>

(it allowed me to not have to write propTypes for CheckboxGroup).

Why would you not want this?

Pure laziness 🐢

oh wait you removed it, nevermind :D

chamkank added 10 commits July 30, 2018 13:25

Create basic checkbox component

d4d12ab

Move Checkbox component to CheckboxGroup

c352859

Use React context API for Checkbox/CheckboxGroup

352cc1e

This lets us only define "name" once (as a prop for CheckboxGroup) and have it be shared by all the child components (Checkboxes).

Allow context for additional Checkbox props (disabled, onChange, and …

978e34c

…defaultChecked)

Remove old checkbox

b10fb9f

Add unstyled checkbox to demo

182ce54

Add checkbox assets

d232518

Add pure CSS checkbox styling

996aa59

Add image-based checkbox styling

0d1e2c3

Fix checkbox label width

20f52f9

chamkank added :web status: wip labels Aug 1, 2018

mingyokim reviewed Aug 1, 2018

View reviewed changes

chamkank added 2 commits August 1, 2018 17:51

Allow Checkbox to render without CheckboxGroup

e9f945f

Add ungrouped checkbox to demo

9f2ec40

bobheadxi mentioned this pull request Aug 2, 2018

Build improvements, asset imports, local static site serving #30

Merged

chamkank added 4 commits August 2, 2018 15:16

Add Checkbox disabled style

0d357dc

Update Checkbox demo

71f0750

Rename context field

b4b25b7

Merge branch 'master' into style-checkbox

4752fc7

chamkank added status: finalized and removed status: wip labels Aug 3, 2018

bobheadxi approved these changes Aug 3, 2018

View reviewed changes

bobheadxi suggested changes Aug 3, 2018

View reviewed changes

chamkank added 2 commits August 2, 2018 20:47

Update lint rules

36d9728

Fix comma-dangle

b095ee3

eslint --fix

8de42cc

bobheadxi suggested changes Aug 3, 2018

View reviewed changes

chamkank added 2 commits August 2, 2018 22:09

Keep destructuring-assignment rule

6a7a061

Use destructuring for CheckboxGroup props

44bcc6d

bobheadxi approved these changes Aug 3, 2018

View reviewed changes

chamkank added 2 commits August 2, 2018 22:58

Remove modified react/prop-types config

755c7de

Add propTypes to CheckboxGroup

de7e183

chamkank merged commit abb7a7b into master Aug 3, 2018

bobheadxi deleted the style-checkbox branch August 3, 2018 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init/style Checkbox & CheckboxGroup #29

init/style Checkbox & CheckboxGroup #29

chamkank commented Aug 1, 2018 •

edited

Loading

mingyokim Aug 1, 2018

bobheadxi Aug 1, 2018

mingyokim Aug 1, 2018

chamkank Aug 1, 2018

mingyokim Aug 1, 2018

chamkank Aug 1, 2018

bobheadxi Aug 1, 2018

chamkank Aug 1, 2018

chamkank Aug 1, 2018 •

edited

Loading

bobheadxi Aug 1, 2018

mingyokim Aug 1, 2018

chamkank Aug 1, 2018

bobheadxi Aug 1, 2018

chamkank commented Aug 3, 2018

bobheadxi left a comment

bobheadxi left a comment

bobheadxi Aug 3, 2018

chamkank Aug 3, 2018

bobheadxi left a comment

bobheadxi Aug 3, 2018

chamkank Aug 3, 2018 •

edited

Loading

bobheadxi Aug 3, 2018

chamkank Aug 3, 2018

bobheadxi Aug 3, 2018

		@@ -0,0 +1,4 @@
		<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">

init/style Checkbox & CheckboxGroup #29

init/style Checkbox & CheckboxGroup #29

Conversation

chamkank commented Aug 1, 2018 • edited Loading

👷 Changes

Usage:

💭 Notes

TODO

🔦 Testing Instructions

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

chamkank Aug 1, 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

chamkank commented Aug 3, 2018

bobheadxi left a comment

Choose a reason for hiding this comment

bobheadxi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobheadxi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chamkank Aug 3, 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

chamkank commented Aug 1, 2018 •

edited

Loading

chamkank Aug 1, 2018 •

edited

Loading

chamkank Aug 3, 2018 •

edited

Loading