feat(switch): Add component #208

bonniezhou · 2018-08-06T21:02:09Z

Fixes #152.

Ripple is modified because the element with the ripple surface (thumb underlay) is different from the ripple activating element (native input).

moog16 · 2018-08-07T06:01:15Z

packages/ripple/README.md

+
+You may want to apply the visual treatment (CSS classes and styles) for a ripple surface on one element, but have its activation rely on a different element. For example, putting a ripple on a `<div>` which will be activated by focusing on a child `<input>` element. We call the visual element the "ripple surface" and the activating element the "ripple activator".
+
+The `initRipple` callback prop will take in an extra `activator` argument for the case where the ripple activator differs from the ripple surface. If the `activator` argument is not provided, the ripple surface will also serve as the ripple activator.


prop will take in --> prop can take in?

moog16 · 2018-08-07T06:02:45Z

packages/ripple/README.md

+    this.props.initRipple(el /* surface */, this.rippleActivatorElement /* activator */);
+  }
+
+  setRippleActivator = (el) => {


We should be telling devs to use React.createRef for these situations.

moog16 · 2018-08-07T06:09:18Z

packages/switch/NativeControl.js

+
+export default class NativeControl extends React.Component {
+  componentDidMount() {
+    this.props.handleChange(this.props.checked);


I don't think you need to call these here since they are initialized in state in index.js

moog16 · 2018-08-07T06:10:15Z

packages/switch/NativeControl.js

+  handleChange = (e) => {
+    const {handleChange, onChange} = this.props;
+    const {checked} = e.target;
+    handleChange(checked);


Can an onChange be passed in that calls handleChange in the parent function? Then you don't need 2 change functions being passed to this component.

moog16 · 2018-08-07T06:10:48Z

packages/switch/NativeControl.js

+  id: PropTypes.string,
+  onChange: PropTypes.func,
+  handleDisabled: PropTypes.func,
+  handleChange: PropTypes.func,


make sure to remove these if they're unnecessary

moog16 · 2018-08-07T06:11:39Z

packages/switch/ThumbUnderlay.js

+            disabled={disabled}
+            handleChange={handleChange}
+            handleDisabled={handleDisabled}
+            setrippleActivatorEl={(el) => {


use React.createRef() here. Also setRippleActivator**

moog16 · 2018-08-07T06:13:56Z

packages/switch/ThumbUnderlay.js

+  }
+
+  get classes() {
+    return classnames('mdc-switch__thumb-underlay', this.props.className);


Is this supposed to have the same this.props.className from the parent? It looks like classes will be duplicated here from index.js. I don't think that is the intention.

I don't think so? The classes from index.js aren't passed down to ThumbUnderlay

Gotcha - you're just not passing any classes from index.js to the ThumbUnderlay component so it seemed like you didn't need this. But I guess it doesn't hurt to have this.

Oh I see what you're saying. ThumbUnderlay is actually receiving props.className from withRipple, so it's necessary. On the other hand NativeControl isn't getting classes from anywhere, so I removed it there!

ah! i didn't even notice :)

moog16 · 2018-08-07T06:15:36Z

packages/switch/index.js

+        <ThumbUnderlay
+          checked={this.state.checked}
+          disabled={disabled}
+          id={id}


is the ID for a aria-label relationship?

Yup! I renamed it nativeControlId to make it clearer

moog16 · 2018-08-07T06:16:36Z

test/screenshot/full-suite.js

  require('./select/screenshot-suite').default,
+  require('./switch/screenshot-suite').default,


codecov-io · 2018-08-08T18:18:53Z

Codecov Report

Merging #208 into master will decrease coverage by 0.12%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
- Coverage   98.86%   98.73%   -0.13%     
==========================================
  Files          23       26       +3     
  Lines         969     1028      +59     
  Branches       98      102       +4     
==========================================
+ Hits          958     1015      +57     
- Misses         11       13       +2

Impacted Files	Coverage Δ
packages/switch/index.js	`100% <100%> (ø)`
packages/switch/NativeControl.js	`100% <100%> (ø)`
packages/ripple/index.js	`96.15% <81.81%> (ø)`	⬆️
packages/switch/ThumbUnderlay.js	`81.81% <81.81%> (ø)`

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 ac0f87f...f82c425. Read the comment docs.

moog16

lint is failing as well. Also i left a comment about this but there is no way for the user to get a hold of the checked value.

moog16 · 2018-08-08T18:31:16Z

packages/switch/ThumbUnderlay.js

+import React from 'react';
+import PropTypes from 'prop-types';
+import classnames from 'classnames';
+import withRipple from '../ripple';


for now we should be using @material/ripple until we move to a monopackage.

But ripple is modified in this PR so it won't work if it uses @material/ripple :(

even if you do lerna add @material/ripple --scope=@material/react-switch?

moog16 · 2018-08-08T18:35:28Z

packages/switch/ThumbUnderlay.js

+    const {handleChange, onChange} = this.props;
+    const {checked} = e.target;
+    handleChange(checked);
+    onChange(e);


I don't think this should be here. You're not passing onChange in index.js to .

Ok makes sense since the user wouldn't have access to the ThumbUnderlay anyways. Removed.

moog16 · 2018-08-08T20:21:21Z

packages/switch/index.js

+export default class Switch extends Component {
+  foundation_ = null;
+  state = {
+    checked: this.props.checked,


oh cool - i didn't know this worked.

moog16 · 2018-08-08T20:33:42Z

packages/switch/README.md

+  render() {
+    return (
+      <Switch nativeControlId='my-switch' />
+      <label for='my-switch'>My Switch</label>


for -> htmlFor

moog16 · 2018-08-08T20:34:08Z

packages/switch/README.md

+import "@material/react-switch/dist/switch.css";
+```
+
+### Javascript Instantiation


how does one get a hold of the value? if its checked/not checked

Added a note to the README, basically same way as in text field

lynnmercier · 2018-08-09T16:35:46Z

packages/ripple/index.js

      const MATCHES = util.getMatchesProperty(HTMLElement.prototype);

      return {
        browserSupportsCssVars: () => util.supportsCssVariables(window),
        isUnbounded: () => this.props.unbounded,
-        isSurfaceActive: () => instance[MATCHES](':active'),
+        isSurfaceActive: () => activator ? activator[MATCHES](':active') : surface[MATCHES](':active'),


this is so simple and lovely. I wonder if we could simplify the Vanilla version of Checkbox and Switch in MDCW to look more like this.
https://github.com/material-components/material-components-web/blob/master/packages/mdc-switch/index.js#L79
https://github.com/material-components/material-components-web/blob/master/packages/mdc-checkbox/index.js#L63

We could probably modify createAdapter to take in an extra param as well

lynnmercier · 2018-08-09T16:36:41Z

packages/ripple/index.js

@@ -148,7 +145,13 @@ const withRipple = (WrappedComponent) => {
      this.setState({style: updatedStyle});
    }

-    getMergedStyles = () => {
+    get classes() {
+      const {className: wrappedCompClasses} = this.props;


wrappedComponentClasses. Don't shorten it

lynnmercier · 2018-08-09T16:38:38Z

packages/switch/NativeControl.js

+  disabled: PropTypes.bool,
+  id: PropTypes.string,
+  onChange: PropTypes.func,
+  setRippleActivator: PropTypes.oneOfType([


can this just be rippleActivator, which I assume is an Element, since it gets passed to a ref

It's a ref which gets this PropType definition according to StackOverflow. Changed the name to rippleActivatorRef to make it clearer.

lynnmercier · 2018-08-09T16:39:44Z

packages/switch/README.md

+
+Prop Name | Type | Description
+--- | --- | ---
+className | String | Classes to be applied to the chip element


hmmm, not a chip

lynnmercier · 2018-08-09T16:46:41Z

packages/switch/index.js

+        classList.delete(className);
+        this.setState({classList});
+      },
+      isNativeControlChecked: () => this.state.checked,


is it weird that we implement isNativeControlChecked by look at the root component's state? I guess we are basically guaranteeing that if the native control's state changes, then ThumbUnderlay will respond in an onChange callback. And then if ThumbUnderlay's state changes we guarantee that Switch's state will change also with and handleChange callback.

But for some reason that feels a bit fragile. Did we do the same design for the text field?

Yep, but text field only has one layer of nesting (is nesting the right word?) TextField -> Input which makes it seem less fragile. And similarly Select->NativeControl. Switch is the only component for now with 3 layers because it needs one for the native input, one for the ripple surface, and one for the top-level component.

lynnmercier · 2018-08-09T16:47:12Z

packages/switch/ThumbUnderlay.js

+  className: PropTypes.string,
+  disabled: PropTypes.bool,
+  nativeControlId: PropTypes.string,
+  handleChange: PropTypes.func,


why not onChange? Why handleChange?

I think on* conventionally expects an event parameter. Confirm, @moog16?

I've been using on* as an event handler. I think we invented handle* so it wouldn't collide with an end developer. But in this case the end dev won't ever pass an onChange method so we can change it to onChange

👍 Updated!

moog16

I think add a test for the Thumbunderlay's onChange={(e) => onChange(e.target.checked)}. But the rest looks good to me! Nice work 🗡

…o best practices

lynnmercier · 2018-08-14T17:01:13Z

I pushed some changes to this PR. But We it now relies on the MDCSwitchFoundation.handleChange method taking an event as an argument. That is implemented in material-components/material-components-web#3342, but it is not merged or released yet. So I think we might have to hold off merging this PR until that PR 3342 is merged.

Or we could do some work around for now? I'd rather wait than think about hacks, but maybe someone has a clever idea

moog16 · 2018-08-15T17:29:52Z

Ya I agree - no sense in making a work around and rush getting this PR in. We can wait until MDCW releases 0.39.0. Then we can update MDCR repo, and get this PR working. Then release 0.5.0

moog16 · 2018-08-28T23:54:43Z

packages/switch/NativeControl.js

+import React from 'react';
+import PropTypes from 'prop-types';
+
+export default class NativeControl extends React.Component {


change this to stateless component:

const NativeControl = (props) => { return (...) } export default NativeControl;

moog16 · 2018-08-28T23:56:16Z

packages/switch/NativeControl.js

+      <input
+        type='checkbox'
+        role='switch'
+        className='mdc-switch__native-control'


Do you think people will use this component on its own? If so we should merge this.props.className and 'mdc-switch__native-control'

moog16 · 2018-08-29T00:00:36Z

packages/switch/index.js

+export default class Switch extends Component {
+  rippleActivator = React.createRef();
+  foundation_ = null;
+  state = {


I think we should use constructor to set initial state:

constructor(props) { super(props); rippleActivator = React.createRef(); foundation_ = null; this.state = { checked: props.checked, classList: new Set(), disabled: props.disabled, nativeControlChecked: props.checked, nativeControlDisabled: props.disabled, }; }

moog16 · 2018-08-29T00:01:25Z

packages/switch/package.json

+  "name": "@material/react-switch",
+  "version": "0.0.0",
+  "description": "Material Components React Switch",
+  "license": "Apache-2.0",


moog16 · 2018-08-29T00:01:34Z

packages/switch/package.json

+  },
+  "dependencies": {
+    "@material/react-ripple": "^0.4.0",
+    "@material/ripple": "^0.38.0",


ripple needs to be 39

actually remove this and only rely on react-ripply

moog16

This doesn't work:

class TestSwitch extends React.Component {
  state = {checked: true};

  render() {
    return (
      <div>
        <Switch checked={this.state.checked} />
        <button onClick={() => this.setState({checked: !this.state.checked})}>
          update
        </button>
      </div>
    )
  }
}

bonniezhou and others added 14 commits August 1, 2018 14:52

first pass

2a3b248

get switch working

6a588db

hallelujah ripple works

064b030

rename surface and activator

16a8280

call ripple on thumb underlay

55980a3

cleanup

85667f2

more cleanup

a867776

clean up ripple

dc545da

Lint and things

126aa49

tests

4657ae1

some fixes

e934421

readme

fbdd2f9

Merge branch 'master' into feat/switch

2e29e58

fix merge

113a090

bonniezhou requested a review from moog16 August 6, 2018 21:12

moog16 suggested changes Aug 7, 2018

View reviewed changes

bonniezhou added 5 commits August 7, 2018 18:40

Use refs

4d2065b

Simple fixes

5046822

Move handleChange

6faaefd

id and readme

30cd87f

Fix tests and all

0d6bb42

lint

1b11581

moog16 suggested changes Aug 8, 2018

View reviewed changes

bonniezhou and others added 6 commits August 8, 2018 20:42

add screenshot test

7dab1a3

npmignore and checked state

023607e

Remove unnecessary internal props

596168f

Merge branch 'master' into feat/switch

4b86c46

readme fix

ed9324d

ripple package

6179d6b

lynnmercier suggested changes Aug 9, 2018

View reviewed changes

bonniezhou added 2 commits August 10, 2018 12:00

Fixes

81d533c

handleChange to onChange

0f28295

moog16 approved these changes Aug 10, 2018

View reviewed changes

bonniezhou and others added 4 commits August 10, 2018 14:31

Add test

a04dc38

lint

ffdceae

Merge branch 'master' into feat/switch

2de3b04

move NativeControl to root element, and update the disabled/checked t…

44603b9

…o best practices

lynnmercier added 7 commits August 28, 2018 13:11

Merge branch 'master' into feat/switch

1149433

Merge branch 'master' into feat/switch

8975a31

merge fixes

7f6741c

updates

4f53d3d

fixes

344f6e5

attempt to fix screenshots

f5e0ee7

Merge branch 'master' into feat/switch

be48874

lynnmercier approved these changes Aug 28, 2018

View reviewed changes

moog16 reviewed Aug 28, 2018

View reviewed changes

moog16 reviewed Aug 29, 2018

View reviewed changes

moog16 suggested changes Aug 29, 2018

View reviewed changes

lynnmercier added 2 commits August 30, 2018 12:41

addressing comments

3ec0a45

fix lint

c248cd2

moog16 approved these changes Aug 30, 2018

View reviewed changes

Update index.js

f82c425

moog16 merged commit 3eb8a4b into master Aug 30, 2018

moog16 deleted the feat/switch branch August 30, 2018 21:27


		You may want to apply the visual treatment (CSS classes and styles) for a ripple surface on one element, but have its activation rely on a different element. For example, putting a ripple on a `<div>` which will be activated by focusing on a child `<input>` element. We call the visual element the "ripple surface" and the activating element the "ripple activator".

		The `initRipple` callback prop will take in an extra `activator` argument for the case where the ripple activator differs from the ripple surface. If the `activator` argument is not provided, the ripple surface will also serve as the ripple activator.

		require('./select/screenshot-suite').default,
		require('./switch/screenshot-suite').default,

feat(switch): Add component #208

feat(switch): Add component #208

Conversation

bonniezhou commented Aug 6, 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

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

codecov-io commented Aug 8, 2018 • edited Loading

Codecov Report

moog16 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

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

moog16 left a comment

Choose a reason for hiding this comment

lynnmercier commented Aug 14, 2018

moog16 commented Aug 15, 2018

moog16 Aug 28, 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

moog16 left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 8, 2018 •

edited

Loading

moog16 Aug 28, 2018 •

edited

Loading