feat(switch): Update switch to use JS and have ripple #2915

rlfriedman · 2018-06-09T02:40:45Z

Addresses #2825

googlebot · 2018-06-09T02:40:49Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

abhiomkar · 2018-06-19T00:12:09Z

demos/theme/index.html

@@ -697,7 +697,7 @@ <h3 class="mdc-typography--headline5 demo-component-section__heading">
          <div class="mdc-switch">
            <input type="checkbox" id="basic-switch" class="mdc-switch__native-control" role="switch" checked>
            <div class="mdc-switch__background">
-              <div class="mdc-switch__knob"></div>
+              <div class="mdc-switch__thumb"></div>


HTML structure needs to be updated here too.

Same below.

Shouldn't the input tag be inside __thumb as per new HTML structure?

Thanks, done!

abhiomkar · 2018-06-19T00:13:33Z

packages/material-components-web/index.js

@@ -62,6 +63,7 @@ autoInit.register('MDCTextField', textField.MDCTextField);
 autoInit.register('MDCMenu', menu.MDCMenu);
 autoInit.register('MDCSelect', select.MDCSelect);
 autoInit.register('MDCSlider', slider.MDCSlider);
+autoInit.register('MDCSwitch', switchComponent.MDCSwitch);


switch.MDCSwitch?

Unfortunately not possible because switch is a reserved word.

abhiomkar · 2018-06-19T00:16:09Z

packages/mdc-switch/README.md

+
+# Switches
+
+<!--<div class="article__asset">


Please remove commented code.

This is used for the site generator: https://github.com/material-components/material-components-site-generator

abhiomkar · 2018-06-19T00:18:04Z

packages/mdc-switch/README.md

+  </a>
+</div>-->
+
+Switches toggle the state of a single settings option on or off, and are mobile preferred.


"Switches toggle the state of a single setting on or off. They are the preferred way to adjust settings on mobile."

from Spec.

abhiomkar · 2018-06-19T00:19:40Z

packages/mdc-switch/README.md

+  <div class="mdc-switch__track"></div>
+  <div class="mdc-switch__thumb-underlay">
+    <div class="mdc-switch__thumb">
+        <input type="checkbox" id="basic-switch" class="mdc-switch__native-control" role="switch">


We might not need to add id attribute for usage.

The ids are used to demonstrate using switches with labels with for="id" attributes, so I think we'll want to leave them.

abhiomkar · 2018-06-19T00:56:37Z

packages/mdc-switch/index.js

+  }
+
+  /** @return {!MDCRipple} */
+  get ripple() {


should this property be public?

There seems to be a public getter exposed for all of the components with ripple, so I went with that as well here.

abhiomkar · 2018-06-19T01:09:58Z

packages/mdc-switch/mdc-switch.scss

+    mdc-switch-transition(background-color),
+    mdc-switch-transition(border-color);
+  border: 1px solid;
+  border-radius: 7px;


$mdc-switch-track-height / 2

abhiomkar · 2018-06-19T01:14:17Z

demos/switch.scss

-  @include mdc-switch-knob-color($color);
-  @include mdc-switch-focus-indicator-color($color);
+
+  @include mdc-switch-toggled-on-track-color($color);


Do you think it makes sense to create a new mixin which packages these three mixins?

mdc-switch-toggle-color($color);

We could do that to make it simple to customize more easily and also do the same for toggled off. Would you be ok with calling the default something like mdc-switch-toggled-on-color? Without the 'on' it might not be clear what the mixin is doing (as it could be referring to either state).

Yah, since the mixin applies color to only ON state it makes sense to name it mdc-switch-toggled-on-color.

Update the docs too.

rlfriedman

I've updated code based on comments and also now made it so that the tap target (input element) covers the whole switch and stays stationary. This means that if you click on the ripple to check/uncheck the switch, you can click in the same area again to reverse that. Previously, once the switch was toggled, that area was no longer clickable.

rlfriedman · 2018-06-21T01:08:24Z

demos/switch.scss

-  @include mdc-switch-knob-color($color);
-  @include mdc-switch-focus-indicator-color($color);
+
+  @include mdc-switch-toggled-on-track-color($color);


We could do that to make it simple to customize more easily and also do the same for toggled off. Would you be ok with calling the default something like mdc-switch-toggled-on-color? Without the 'on' it might not be clear what the mixin is doing (as it could be referring to either state).

rlfriedman · 2018-06-21T01:15:10Z

packages/mdc-switch/README.md

+
+# Switches
+
+<!--<div class="article__asset">


This is used for the site generator: https://github.com/material-components/material-components-site-generator

rlfriedman · 2018-06-21T01:15:32Z

packages/mdc-switch/README.md

+  <div class="mdc-switch__track"></div>
+  <div class="mdc-switch__thumb-underlay">
+    <div class="mdc-switch__thumb">
+        <input type="checkbox" id="basic-switch" class="mdc-switch__native-control" role="switch">


The ids are used to demonstrate using switches with labels with for="id" attributes, so I think we'll want to leave them.

rlfriedman · 2018-06-21T01:18:51Z

demos/theme/index.html

@@ -697,7 +697,7 @@ <h3 class="mdc-typography--headline5 demo-component-section__heading">
          <div class="mdc-switch">
            <input type="checkbox" id="basic-switch" class="mdc-switch__native-control" role="switch" checked>
            <div class="mdc-switch__background">
-              <div class="mdc-switch__knob"></div>
+              <div class="mdc-switch__thumb"></div>


rlfriedman · 2018-06-21T01:19:07Z

packages/mdc-switch/README.md

+  </a>
+</div>-->
+
+Switches toggle the state of a single settings option on or off, and are mobile preferred.


rlfriedman · 2018-06-21T01:19:30Z

packages/mdc-switch/mdc-switch.scss

+    mdc-switch-transition(background-color),
+    mdc-switch-transition(border-color);
+  border: 1px solid;
+  border-radius: 7px;


abhiomkar · 2018-06-21T21:03:36Z

demos/switch.scss

-  @include mdc-switch-knob-color($color);
-  @include mdc-switch-focus-indicator-color($color);
+
+  @include mdc-switch-toggled-on-track-color($color);


Yah, since the mixin applies color to only ON state it makes sense to name it mdc-switch-toggled-on-color.

Update the docs too.

abhiomkar · 2018-06-21T21:07:33Z

demos/theme/index.html

@@ -697,7 +697,7 @@ <h3 class="mdc-typography--headline5 demo-component-section__heading">
          <div class="mdc-switch">
            <input type="checkbox" id="basic-switch" class="mdc-switch__native-control" role="switch" checked>
            <div class="mdc-switch__background">
-              <div class="mdc-switch__knob"></div>
+              <div class="mdc-switch__thumb"></div>


Shouldn't the input tag be inside __thumb as per new HTML structure?

abhiomkar · 2018-06-21T21:23:05Z

packages/mdc-switch/adapter.js

@@ -0,0 +1,53 @@
+/**
+ * @license
+ * Copyright 2016 Google Inc. All Rights Reserved.


abhiomkar · 2018-06-21T21:24:17Z

packages/mdc-switch/constants.js

+/** @enum {string} */
+const strings = {
+  NATIVE_CONTROL_SELECTOR: `.${ROOT}__native-control`,
+  RIPLE_SURFACE_SELECTOR: `.${ROOT}__thumb-underlay`,


nit: RIPPLE_SURFACE_SELECTOR*

Thanks, done!

abhiomkar · 2018-06-21T21:38:37Z

packages/mdc-switch/foundation.js

@@ -0,0 +1,118 @@
+/**


Update README for MDCSwitchFoundation & MDCSwitchAdapter.

Good catch! Updated.

…king the native control in the foundation

patrickrodee · 2018-06-26T01:12:13Z

packages/mdc-switch/_mixins.scss

+@import "./variables";
+
+@mixin mdc-switch-toggled-on-color($color) {


These are all super clean. Nice!!!

patrickrodee · 2018-06-26T01:13:17Z

packages/mdc-switch/_variables.scss

+
+$mdc-switch-baseline-theme-color: secondary;
+
+$mdc-switch-thumb-vertical-offset_: -3px;


Could you add a comment explaining why this is a negative value?

Will do, I think this was more of a remnant from before and I think I can just add this into the top value for the underlay instead.

patrickrodee · 2018-06-26T01:14:02Z

packages/mdc-switch/adapter.js

+ */
+
+/* eslint-disable no-unused-vars */
+import {MDCSelectionControlState} from '@material/selection-control/index';


Where in this file is MDCSelectionControlState used?

Good catch, no longer being used here or in foundation.js.

patrickrodee · 2018-06-26T01:15:37Z

packages/mdc-switch/constants.js

+
+/** @enum {string} */
+const strings = {
+  NATIVE_CONTROL_SELECTOR: `.${ROOT}__native-control`,


Since you don't use the ROOT constant in the cssClasses, I'd be tempted to remove it and just have these be normal strings instead of template strings. It's not exported either so it's not doing much.

patrickrodee · 2018-06-26T01:18:34Z

packages/mdc-switch/foundation.js

+  /** @return {!MDCSwitchAdapter} */
+  static get defaultAdapter() {
+    return /** @type {!MDCSwitchAdapter} */ ({
+      addClass: (/* className: string */) => {},


I'm not sure if these annotations for faux-parameter types are necessary. Does Closure still pass if they're removed?

I think it would yes, but I was just basing this on looking at most of the other foundation get defaultAdapter methods which do this and assumed this was the preferred style. Do you know if it's cool to move away from this now or if there's a reason the other files are doing it?

AFAIK, these are not closure annotations :)

We might be duplicating the defining the types in foundation & adapter file.

patrickrodee · 2018-06-26T01:21:44Z

packages/mdc-switch/foundation.js

+  /** @param {boolean} disabled */
+  setDisabled(disabled) {
+    this.adapter_.setDisabled(disabled);
+    disabled ? this.adapter_.addClass(cssClasses.DISABLED) : this.adapter_.removeClass(cssClasses.DISABLED);


I'm not a huge fan of these ternaries. I'd suggest making it an if statement with an early-exit return or a normal if/else. I totally get that's just a reworking of what this does. I just find it easier to read.

I'm fine either way, so will switch to if/else.

patrickrodee · 2018-06-26T01:25:47Z

packages/mdc-switch/foundation.js

+   * Updates the styling of the switch based on its checked state.
+   * @private
+   */
+  updateCheckedStyling_() {


I'd suggest giving this a isChecked boolean argument and rewriting the check to use an if with early return or if/else flow. Then, moving the this.isChecked() call into handleChange() since that seems to be the only place where you're uncertain of the checked state. You can then update setChecked to pass the checked argument into updateCheckedStyling_ directly.

Let me know what you think!

abhiomkar

LGTM.

Please also add a screenshot test for switch in separate PR.

abhiomkar · 2018-06-26T20:55:00Z

demos/switch.html

@@ -138,6 +147,12 @@ <h2>Disabled</h2>
      document.getElementById('toggle-rtl').addEventListener('change', function() {
        this.checked ? demoWrapper.setAttribute('dir', 'rtl') : demoWrapper.removeAttribute('dir');
      });
+      demoReady(function() {
+        const switches = document.querySelectorAll('.mdc-switch');


const => var

We usually use ES5 here since the demo pages are not compiled.

Sounds good

abhiomkar · 2018-06-26T21:07:19Z

packages/mdc-switch/adapter.js

+ * limitations under the License.
+ */
+
+/* eslint-disable no-unused-vars */


This comment line may no longer be required.

Will remove

Actually a version of this is still required, otherwise it complains about the unused params.

abhiomkar · 2018-06-26T21:09:01Z

packages/mdc-switch/foundation.js

+  /** @return {!MDCSwitchAdapter} */
+  static get defaultAdapter() {
+    return /** @type {!MDCSwitchAdapter} */ ({
+      addClass: (/* className: string */) => {},


AFAIK, these are not closure annotations :)

We might be duplicating the defining the types in foundation & adapter file.

abhiomkar · 2018-06-26T21:16:44Z

packages/mdc-switch/adapter.js

+  isChecked() {}
+
+  /** @param {boolean} disabled */
+  setDisabled(disabled) {}


Suggestion: should we name it setNativeDisabled()?

it might get confused with the Foundation's setDisabled() method. WDYT?

That's a good point, perhaps (although a bit longer) it should be setNativeControlChecked/setNativeControlDisabled as to match other components with native control elements.

abhiomkar · 2018-06-26T21:33:14Z

packages/mdc-switch/mdc-switch.scss

+  display: flex;
+  position: absolute;
+  // Ensures the knob is centered on the track.
+  top: -17px;


Mathematically this is -($mdc-switch-track-height / 2 + $mdc-switch-thumb-diameter / 2) :)

The distance between the center of the track and the thumb relative to its original position.

I can switch to that to be more clear!

switchControl.

patrickrodee

I'm all about it. My only change is to remove the [de]registerChangeHandler methods from the adapter. That will require registering and deregistering the listeners manually in the index.js/MDCComponent file.

There's a good example in the single-selection list item PR showing what that looks like.

patrickrodee · 2018-07-09T18:57:01Z

packages/material-components-web/README.md

@@ -26,6 +26,7 @@ const checkbox = new mdc.checkbox.MDCCheckbox(document.querySelector('.mdc-check
 import { checkbox } from 'material-components-web';
 const checkbox = new checkbox.MDCCheckbox(document.querySelector('.mdc-checkbox'));
 ```
+> NOTE: Since switch is a reserved word in JS, switch is instead named `switchControl`. 


Nice explanation 👍

patrickrodee · 2018-07-09T18:58:38Z

packages/mdc-switch/README.md

+| `removeClass(className: string) => void` | Removes a class from the root element. |
+| `registerChangeHandler(handler: EventListener) => void` | Registers an event handler to be called when a `change` event is triggered on the native control. |
+| `deregisterChangeHandler(handler: EventListener) => void` | Deregisters an event handler that was previously passed to `registerChangeHandler`. |
+| `setChecked(checked: boolean)` | Sets the checked state of the native control. |


I think these need to be updated to use the new setNativeControlChecked etc.

Nice catch, thanks!

patrickrodee · 2018-07-09T19:02:07Z

packages/mdc-switch/index.js

+      removeClass: (className) => this.root_.classList.remove(className),
+      registerChangeHandler: (handler) => this.nativeControl_.addEventListener('change', handler),
+      deregisterChangeHandler: (handler) => this.nativeControl_.removeEventListener('change', handler),
+      setNativeControlChecked: (checked) => this.nativeControl_.checked = checked,


I like these a lot 🔥

patrickrodee

One last suggestion, otherwise LGTM

patrickrodee · 2018-07-10T20:18:20Z

packages/mdc-switch/README.md

+If you are using a JavaScript framework, such as React or Angular, you can create a switch for your framework. Depending on your needs, you can use the _Simple Approach: Wrapping MDC Web Vanilla Components_, or the _Advanced Approach: Using Foundations and Adapters_. Please follow the instructions [here](../../docs/integrating-into-frameworks.md).
+
+## Event Handlers
+If wrapping the switch component it is necessary to add an event handler for native control change events that calls the `handleChange` foundation method. For an example of this, see the [MDCSwitch](index.js) component `initialSyncWithDOM` method.


I'd suggest making a table similar to the MDCSwitchAdapter one. Something like this:

Event Element Selector Foundation Handler

"change" .mdc-switch__native-control handleChange()

and maybe move it below the foundation section since it's it's related to the foundation.

patrickrodee

🔥🔥🔥

…rial-components-web into feat/new-switch

codecov-io · 2018-07-11T21:06:49Z

Codecov Report

Merging #2915 into feat/switch-update will increase coverage by <.01%.
The diff coverage is 98.8%.

@@                  Coverage Diff                   @@
##           feat/switch-update    #2915      +/-   ##
======================================================
+ Coverage               98.48%   98.49%   +<.01%     
======================================================
  Files                      98      101       +3     
  Lines                    4232     4316      +84     
  Branches                  538      540       +2     
======================================================
+ Hits                     4168     4251      +83     
- Misses                     64       65       +1

Impacted Files	Coverage Δ
packages/mdc-switch/constants.js	`100% <100%> (ø)`
packages/mdc-switch/foundation.js	`100% <100%> (ø)`
packages/mdc-switch/index.js	`98% <98%> (ø)`

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 e3e400d...cbae6f6. Read the comment docs.

rlfriedman and others added 7 commits May 31, 2018 11:04

WIP: Initial switch with JS

1c0b150

WIP: Actually add switch JS

496fa99

WIP: Switch mixin updates

9c6d942

WIP: Start adding tests

183043c

WIP: Test and various other updates

dce1c7a

WIP: Updates to switch styling, HTML, and docs

df41520

WIP: Some small updates

102f778

abhiomkar requested changes Jun 19, 2018

View reviewed changes

WIP: Make switch click target cover whole switch and respond to comments

3b68633

rlfriedman commented Jun 21, 2018

View reviewed changes

abhiomkar requested changes Jun 21, 2018

View reviewed changes

rlfriedman added 2 commits June 25, 2018 17:44

WIP: Respond to comments and add new adapter methods to separate chec…

a805eb4

…king the native control in the foundation

WIP: Remove unnecessary include

afe06b7

patrickrodee requested changes Jun 26, 2018

View reviewed changes

WIP: Code review updates.

5bad51e

abhiomkar approved these changes Jun 26, 2018

View reviewed changes

patrickrodee self-assigned this Jun 27, 2018

WIP: Rename native control adapter methods and switchComponent to

658993a

switchControl.

patrickrodee requested changes Jul 9, 2018

View reviewed changes

WIP: Remove change handler (de)registration from adapter.

ac1939f

patrickrodee requested changes Jul 10, 2018

View reviewed changes

rlfriedman added 4 commits July 10, 2018 14:08

WIP: Add a table for event handlers.

14e5365

WIP: Add missing space in table

8d1d350

WIP: Small formatting change

20ed158

WIP: Try to fix table formatting

c043085

patrickrodee approved these changes Jul 10, 2018

View reviewed changes

rlfriedman added 3 commits July 11, 2018 13:16

Merge branch 'feat/switch-update' into feat/new-switch

3029022

WIP: Attempt to fix tests on IE

89416cd

Merge branch 'feat/new-switch' of github.com:material-components/mate…

cbae6f6

…rial-components-web into feat/new-switch

rlfriedman merged commit bedd674 into feat/switch-update Jul 11, 2018

		@import "./variables";

		@mixin mdc-switch-toggled-on-color($color) {


		$mdc-switch-baseline-theme-color: secondary;

		$mdc-switch-thumb-vertical-offset_: -3px;

feat(switch): Update switch to use JS and have ripple #2915

feat(switch): Update switch to use JS and have ripple #2915

Conversation

rlfriedman commented Jun 9, 2018

googlebot commented Jun 9, 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

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

rlfriedman 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

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

abhiomkar 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

patrickrodee 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

patrickrodee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickrodee left a comment

Choose a reason for hiding this comment

codecov-io commented Jul 11, 2018

Codecov Report