refactor(select): replace menu with native html select #2462

moog16 · 2018-03-26T21:44:28Z

moog16 · 2018-03-26T22:09:39Z

packages/mdc-select/index.js

  }

-  nameditem(key) {


may want to reimplement namedItem

…al-components/material-components-web into refactor/select/use-native-select

lynnmercier · 2018-03-27T18:39:43Z

packages/mdc-select/README.md

 </div>
 ```

+Use the `<option disabled selected/>` element as a way to display the select having no value.


lynnmercier · 2018-03-27T18:46:25Z

packages/mdc-select/README.md


 | Method Signature | Description |
 | --- | --- |
 | `item(index: number) => HTMLElement?` | Analogous to `HTMLSelectElement.prototype.item`. Returns the option at the specified index, or `null` if the index is out of bounds. }
 | `nameditem(key: string) => HTMLElement?` | Analogous to `HTMLSelectElement.prototype.nameditem`. Returns the options either whose `id` equals the given `key`, or whose `name` attribute equals the given `key`. Returns `null` if no item with an `id` or `name` attribute of the specified key is found. |
+| `initialSyncWithDOM() => void` | Syncs the component with the current state of the HTML markup. |


This is a method on all components, you don't need to document it

lynnmercier · 2018-03-27T18:47:18Z

packages/mdc-select/README.md

-| `mdc-list-divider`       | A divider.                                      |
-
-It is advised that dividers also set `role="presentation"` to disable selection and not cloud accessibility.
+| `mdc-select__option`     | A select option.                                |


Is this a necessary class?

…ethods on foundation

…al-components/material-components-web into refactor/select/use-native-select

codecov-io · 2018-03-27T23:44:55Z

Codecov Report

Merging #2462 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2462      +/-   ##
==========================================
- Coverage    98.9%   98.87%   -0.04%     
==========================================
  Files         102      102              
  Lines        4218     4073     -145     
  Branches      528      510      -18     
==========================================
- Hits         4172     4027     -145     
  Misses         46       46

Impacted Files	Coverage Δ
packages/mdc-select/foundation.js	`100% <100%> (ø)`	⬆️
packages/mdc-select/index.js	`100% <100%> (ø)`	⬆️
packages/mdc-ripple/util.js	`100% <100%> (ø)`	⬆️
packages/mdc-select/constants.js	`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 a70aa88...c816555. Read the comment docs.

williamernest · 2018-03-27T23:53:14Z

packages/mdc-select/mdc-select.scss


  border-bottom-width: 1px;
  border-bottom-style: dotted;
  opacity: .38;
  cursor: default;
  pointer-events: none;
-  // Imitate native disabled functionality
-  user-select: none;

  .mdc-select__bottom-line {
    display: none;
  }
 }

 // postcss-bem-linter: end


Do we still need this?

williamernest · 2018-03-28T00:27:41Z

packages/mdc-select/index.js

-    return this.root_.querySelectorAll('[aria-selected]');
+    // In an array because HTML spec also returns an array.
+    // Cannot access HTMLSelectElement.selectedIndex since it is not supported by IE11.
+    // https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement/selectedIndex


selectedIndex -> selectedOptions
https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement/selectedIndex -> https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement/selectedOptions

kfranqueiro · 2018-03-28T17:32:45Z

I'll get to this review in a bit, but FYI I'm immediately noticing that the arrow doesn't look like it's vertically aligned correctly for the base MDC Select...

kfranqueiro

First round of review. Didn't bother reviewing tests because I have some adapter API suggestions first.

kfranqueiro · 2018-03-28T17:30:08Z

demos/select.html

          </div>
        </section>
      </section>
+      <section class="example">
+        <h2 class="mdc-typography--title">OptGroup JS Component</h2>


Re-word to "MDC Select with optgroups"

kfranqueiro · 2018-03-28T19:01:54Z

packages/mdc-select/README.md

-option with `aria-selected`. This will ensure that the label moves out of the way of the select's value
-and prevents a Flash Of Un-styled Content (**FOUC**).
+option with the `selected` attribute. This will ensure that the label moves out
+of the way of the select's value and prevents a Flash Of Un-styled Content (**FOUC**).


While we're here, remove the - from unstyled

kfranqueiro · 2018-03-28T19:21:53Z

packages/mdc-select/README.md

 </div>
 ```

+#### Select with floating label as the placeholder
+
+Use the `<option disabled selected hidden/>` element as a way to display the select having no value.


hidden seems to have issues even in some browsers that support it, e.g. Edge and Safari where it leaves an empty space in the menu because of the hidden option. Between that and it not being supported by IE at all, I'd be inclined to leave it out...

Also, I'm not sure why you're saying to add the float-above class here? If the select initially has "no value", you would not want the float-above modifier (and our demo seems to match that).

Aside from that, I think this paragraph could use some re-wording anyway:

By default, `<select>` elements will select their first enabled option. In order to initially display a placeholder instead, add an initial `<option>` element with the `disabled` *and* `selected` attributes set, and with `value` set to `""`.

I was saying to add float-above if you want a placeholder instead of using the label, but the way I worded it is confusing and this might not be the place so I'll remove. Should I also remove hidden from the demos as well?

Yes, I think I commented on the line in the demo page too.

Also, not sure what you mean regarding wanting a placeholder instead of using the label... the placeholder attribute is not a thing for native select (since select normally always has an option selected)

kfranqueiro · 2018-03-28T19:22:13Z

packages/mdc-select/README.md

+if you do use this option as a placeholder to add the `mdc-select__label--float-above` class. Refer to [pre-selected option](#select-with-pre-selected-option). Also note that not all browsers respect the `hidden` attribute (ie. IE11).
+
+```html
+<option value="" disabled selected hidden></option>


Remove hidden as per above comment

kfranqueiro · 2018-03-28T19:23:15Z

packages/mdc-select/README.md

 #### Disabled select

+Add the `mdc-select--disabled` class to the `mdc-select` element. The disabled


I would re-word this to always recommend also setting the disabled attribute on the select element, again to prevent FOUC (and more importantly to prevent users from interacting with the select if JS loads slowly). The initialSyncWithDOM stuff should be more of a coincidental implementation detail that isn't needed here.

Also, throughout the readme, don't refer to <select> and <option> with slashes... they always have matching closing tags, so this just seems unnatural.

kfranqueiro · 2018-03-28T20:53:57Z

packages/mdc-select/mdc-select.scss

-    display: flex;
-    position: relative;
-    flex-grow: 1;
+    &::-ms-expand {


Above this, also add @include mdc-typography-base; to force consistent font (browsers like to reset font on inputs, and the box variant already resolves this by way of including a specific typography scale which itself includes base)

This still needs to be resolved; non-box selects are still using the browser's default input font

kfranqueiro · 2018-03-28T20:57:54Z

packages/mdc-select/index.js


-    this.ripple = new MDCRipple(this.surface_);
+    this.ripple = new MDCRipple(this.root_);


Two issues here:

We probably want to do something like what we do with checkbox/radio/text-field to forward the ripple component's event handlers to the select element. This should make activation work when pressing space, and should make focus styles work via ripple JS.

We should only instantiate ripple if we're using the box variant (text field should have similar logic to check for a modifier class).

kfranqueiro · 2018-03-28T21:05:47Z

packages/mdc-select/mdc-select.scss

@@ -114,14 +109,15 @@
    background-position: left 10px center;
  }

-  .mdc-select__surface {
+  .mdc-select__surface,


Since this element is no longer the ripple surface, we might want to consider renaming it... e.g. checkbox has __native-control for this.

(We'd want to change variable names in JS too)

kfranqueiro · 2018-03-28T21:06:58Z

packages/mdc-select/mdc-select.scss

@@ -27,12 +28,15 @@
 // postcss-bem-linter: define select
 .mdc-select {
  @include mdc-typography-base;
+  @include mdc-states;


This should be in --box styles where the other ripple mixins are used. It doesn't make sense for this to apply to anything without those styles.

When you move it, change it to: @include mdc-states(text-primary-on-light, $has-nested-focusable-element: true);

This will make the ripple match the ink color, and make focus shade work properly given the nested select element, even when ripple JS isn't used (I know, right now we require JS, but this seems like the more technically correct thing to do esp. if there is ever hope of CSS-only down the road)

kfranqueiro · 2018-03-28T21:08:17Z

packages/mdc-select/mdc-select.scss

@@ -27,12 +28,15 @@
 // postcss-bem-linter: define select
 .mdc-select {
  @include mdc-typography-base;
+  @include mdc-states;
+


We might want to go ahead and remove the following @each which is applying letter-spacing typography styles, since IIUC we're probably removing that property from typography anyway, and this might be hard to catch when we do a sweep later... @lynnjepsen what do you think?

kfranqueiro

Looking good RE APIs; reviewed tests this time too. I think the arrow location hasn't been fixed for non-box yet?

kfranqueiro · 2018-03-30T14:31:28Z

demos/select.html

+          <div id="box-js-select" class="mdc-select mdc-select--box">
+            <select class="mdc-select__native-control">
+              <option value="" disabled selected></option>
+              <option value="fruit-roll-ups1">


These don't need "1"s on the values anymore, thanks to us no longer awkwardly using IDs as values. (Don't forget to update the value set in the JS below too)

kfranqueiro · 2018-03-30T14:32:15Z

demos/select.html

-            </div>
+          <div id="js-pre-selected" class="mdc-select">
+            <select class="mdc-select__native-control">
+              <option value="fruit-roll-ups2" selected>


You can remove the numbers on these values too now.

kfranqueiro · 2018-03-30T14:32:35Z

demos/select.html

+        <div id="optgroup-js-select" class="mdc-select">
+          <select class="mdc-select__native-control">
+            <optgroup label="Meats">
+              <option value="hot-dog">Steak</option>


hot-dog -> steak?

kfranqueiro · 2018-03-30T14:33:29Z

demos/select.html

+              <option value="hamburger">Hamburger</option>
+            </optgroup>
+            <optgroup label="Vegetables">
+              <option value="beets">Beets</option>


Nit: make this singular to match other entries

kfranqueiro · 2018-03-30T14:39:01Z

packages/mdc-select/mdc-select.scss

    white-space: nowrap;
+    cursor: pointer;
+    appearance: none;


Is the overflow: hidden below this needed? It was needed for __surface before because that's what contained the ripple, but that's been moved.

kfranqueiro · 2018-03-30T15:21:18Z

packages/mdc-select/README.md

+| `getSelectedIndex() => number` | Returns the selected index of the `<select>` element. |
+| `setSelectedIndex(index: number) => void` | Sets the select's selectedValue to the option found at the provided index. If the index is out of the select's range, it will default to -1. |
+| `getValue() => string` | Returns the value selected on the `<select>` element. |
+| `setValue(value: string) => void` | Sets the select's value. If no option has the provided value, it sets `value` to empty string. |


Reduce this to: Sets the value of the <select> element.

(The rest is re-documenting native behavior.)

kfranqueiro · 2018-03-30T15:22:34Z

packages/mdc-select/README.md

+
+| Method Signature | Description |
+| --- | --- |
+| `setValue(value: string) => void` | Sets the select's value. If value is not found within list of options, it will set it to empty string. |


Similar to above, reduce description: Sets the value of the component.

kfranqueiro · 2018-03-30T15:24:58Z

packages/mdc-select/README.md

 </div>
 ```

+#### Select with floating label as the placeholder
+


Extra newlines here and before the next heading? Just need one empty line

kfranqueiro · 2018-03-30T15:31:50Z

packages/mdc-select/mdc-select.scss

-    display: flex;
-    position: relative;
-    flex-grow: 1;
+    &::-ms-expand {


This still needs to be resolved; non-box selects are still using the browser's default input font

kfranqueiro · 2018-03-30T15:34:01Z

packages/mdc-select/index.js


-    this.ripple = new MDCRipple(this.surface_);
+    if (this.root_.classList.contains(cssClasses.BOX)) {
+      this.ripple = new MDCRipple(this.root_);


Only one of my two previous comments was resolved. Here's the other one:

We probably want to do something like what we do with checkbox/radio/text-field to forward the ripple component's event handlers to the select element. This should make activation work when pressing space, and should make focus styles work via ripple JS.

Specifically, I'm talking about e.g. this:

material-components-web/packages/mdc-checkbox/index.js

Lines 64 to 65 in 3f23821

registerInteractionHandler: (type, handler) => this.nativeCb_.addEventListener(type, handler),

deregisterInteractionHandler: (type, handler) => this.nativeCb_.removeEventListener(type, handler),

…ial-components/material-components-web into refactor/select/use-native-select

* Simplify ripple tests * Don't worry about wrapping ripple activation * README fixes

kfranqueiro

I applied one last round of edits this morning; I think this is good, but you might want to look at my changes.

This should probably be feat(select) since it does affect APIs and functionality, and we need something like BREAKING CHANGE: The template and adapter APIs have changed to take advantage of the native select element; see the MDC Select README for more information.

These review points have been resolved

bcardiff · 2018-04-06T17:43:18Z

packages/mdc-select/mdc-select.scss

    white-space: nowrap;
-    overflow: hidden;
+    cursor: pointer;
+    appearance: none;


FYI, across the code base the prefixed version of appearance is used

-moz-appearance: none; -webkit-appearance: none;

According to https://caniuse.com/#search=appearance the unprefixed version has very little support currently

The vendor prefixes are added automatically by autoprefixer when we build releases (and you are encouraged to do the same if you have your own Sass workflow). I have noticed that we have a few components using the vendor prefixes directly, which we should probably fix to use the non-prefixed version that will be picked up by autoprefixer.

I see, thanks for clarifying @kfranqueiro

Is there a documentation for how one is supposed to use @material/* scss as and then apply postcss? Since postcss supports plugins/etc I am not sure which setup is the one mcw rely on.

In my case we compile a css with @material/* mixins to build a custom theme + some overrides.

Change as per material-components-web v0.34.1. reference: material-components/material-components-web#2462 BREAKING CHANGE: - The template and adapter APIs have changed to take advantage of the native select element; see the MDC Select README for more information. - Removed mdc-select-item component. Replace with HTML5's native option element.

Matt Goo added 9 commits March 19, 2018 14:02

WIP: select is workig without menu

b0435e2

WIP: checking tap target IE

fd7b19f

Merge branch 'master' into refactor/select/use-native-select

a9ca540

Merge branch 'master' into refactor/select/use-native-select

2bd3f84

WIP: fixed select box styles and animations

15e8d78

WIP: select readmes

5264eef

fix(select): updated tests

06ace26

WIP: test update

440a652

Mrge branch 'master' into refactor/select/use-native-select

ed4a6e8

moog16 commented Mar 26, 2018

View reviewed changes

packages/mdc-select/index.js Outdated

}

nameditem(key) {

Copy link

Contributor Author

moog16 Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may want to reimplement namedItem

Matt Goo added 2 commits March 26, 2018 15:12

WIP: removed unintentional changes

47ac811

Merge branch 'master' into refactor/select/use-native-select

1188e32

moog16 assigned williamernest Mar 27, 2018

Matt Goo added 2 commits March 26, 2018 17:08

WIP: removed aria selected and aria disabled

8eafe05

Merge branch 'refactor/select/use-native-select' of github.com:materi…

ed42b9f

…al-components/material-components-web into refactor/select/use-native-select

williamernest mentioned this pull request Mar 27, 2018

Store values from checkboxes in MDCSelect #2470

Closed

Matt Goo added 3 commits March 27, 2018 09:27

Merge branch 'master' into refactor/select/use-native-select

9f9c738

fix: test warnings

6309356

fix: IE 11 fix

214fbba

lynnmercier previously requested changes Mar 27, 2018

View reviewed changes

Matt Goo added 5 commits March 27, 2018 12:18

fix: removed __option selector

401a2a2

fix: fixed select in IE since selectedIndex isn't supported

388f261

Merge branch 'master' into refactor/select/use-native-select

196e0f1

fix: removed getters from foundation and converted set methods to m…

c5f2614

…ethods on foundation

Merge branch 'refactor/select/use-native-select' of github.com:materi…

6cce4ad

…al-components/material-components-web into refactor/select/use-native-select

williamernest reviewed Mar 28, 2018

View reviewed changes

fix: comment update

9d0fbf4

moog16 assigned kfranqueiro and unassigned williamernest Mar 28, 2018

kfranqueiro suggested changes Mar 28, 2018

View reviewed changes

WIP: decreased complexity of select adapter, fixed up readme

5fab6ac

kfranqueiro suggested changes Mar 30, 2018

View reviewed changes

Matt Goo and others added 5 commits March 30, 2018 13:56

WIP: fixed tests and readme

0ffd8da

Merge branch 'master' into refactor/select/use-native-select

7c0ef6e

WIP: tests passing!

c4c9ac0

Mterge branch 'refactor/select/use-native-select' of github.com:mater…

b13a01d

…ial-components/material-components-web into refactor/select/use-native-select

WIP Final review revisions

137cb5d

* Simplify ripple tests * Don't worry about wrapping ripple activation * README fixes

kfranqueiro approved these changes Apr 2, 2018

View reviewed changes

WIP Conditionally skip tests dependent on ripple JS (fixes IE11)

c816555

moog16 merged commit fcc7341 into master Apr 2, 2018

moog16 deleted the refactor/select/use-native-select branch April 2, 2018 16:32

moog16 mentioned this pull request Apr 2, 2018

MDC-Select custom max-height is not handled correctly #2284

Closed

acdvorak mentioned this pull request Apr 2, 2018

mdc-text-field in mdc-menu doesn't accept spaces #2471

Closed

This was referenced Apr 2, 2018

MDC Select - no longer form input compatible #2221

Closed

Investigate how Select Menu should select the first matching item when the user types on the keyboard #1961

Closed

bcardiff reviewed Apr 6, 2018

View reviewed changes

trimox mentioned this pull request Apr 7, 2018

feat(select): Replace menu with native html select trimox/angular-mdc-web#834

Merged

acdvorak mentioned this pull request Apr 9, 2018

Select native-control font-size is too small #2538

Closed

		#### Disabled select

		Add the `mdc-select--disabled` class to the `mdc-select` element. The disabled


		this.ripple = new MDCRipple(this.surface_);
		this.ripple = new MDCRipple(this.root_);

	registerInteractionHandler: (type, handler) => this.nativeCb_.addEventListener(type, handler),
	deregisterInteractionHandler: (type, handler) => this.nativeCb_.removeEventListener(type, handler),

refactor(select): replace menu with native html select #2462

refactor(select): replace menu with native html select #2462

Conversation

moog16 commented Mar 26, 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

codecov-io commented Mar 27, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro commented Mar 28, 2018

kfranqueiro 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

kfranqueiro 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

kfranqueiro 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

codecov-io commented Mar 27, 2018 •

edited

Loading