Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

feat(select): Add new UX styles and behavior to select #1097

Merged
merged 38 commits into from
Dec 5, 2017

Conversation

amsheehan
Copy link
Contributor

resolves #674

@codecov-io
Copy link

codecov-io commented Aug 8, 2017

Codecov Report

Merging #1097 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1097      +/-   ##
==========================================
+ Coverage   99.65%   99.65%   +<.01%     
==========================================
  Files          75       75              
  Lines        3459     3494      +35     
  Branches      425      426       +1     
==========================================
+ Hits         3447     3482      +35     
  Misses         12       12
Impacted Files Coverage Δ
packages/mdc-select/constants.js 100% <ø> (ø) ⬆️
packages/mdc-select/foundation.js 100% <100%> (ø) ⬆️
packages/mdc-select/index.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 f7f1eb0...4d1ba25. Read the comment docs.

</section>


Copy link
Contributor

Choose a reason for hiding this comment

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

Is this extra blank line intentional?

@@ -141,19 +136,25 @@ <h2 class="mdc-typography--title">Fully-Featured Component</h2>
<label for="disabled">Disabled</label>
</div>
</section>


Copy link
Contributor

Choose a reason for hiding this comment

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

Is this second blank line intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not. Assume all extraneous white space is a d'oh on my part :)

var heroSelect = document.getElementById('hero-js-select');
mdc.select.MDCSelect.attachTo(heroSelect);
})();
</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC, why do you have two separate script tags and IIFEs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make it easier to copy and paste for people who want to do that.

@@ -179,37 +180,42 @@ <h2 class="mdc-typography--title">CSS Only</h2>
<script>
(function() {
var MDCSelect = mdc.select.MDCSelect;
var root = document.getElementById('js-select');
var currentlySelected = document.getElementById('currently-selected');
var heroSelect = document.getElementById('hero-js-select');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be using const/let instead of var?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not in demo pages, which don't get transpiled and still need to work in all browsers we support.

Copy link
Contributor

Choose a reason for hiding this comment

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

IE 11 (mostly) supports const/let, so which browsers are we concerned about? Safari 9.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if everything fully supported es2015 syntax overnight, I don't think it would be a very good use of time to re-implement the demos just for the new syntactic sugars available to us. As delicious as they may be. :)

@@ -91,8 +91,10 @@ export default class MDCSimpleMenuFoundation extends MDCFoundation {
this.keyupHandler_ = (evt) => this.handleKeyboardUp_(evt);
/** @private {function(!Event)} */
this.documentClickHandler_ = (evt) => {
this.adapter_.notifyCancel();
this.close(evt);
if (!evt.target.classList.contains('mdc-list-item')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to check the ancestor nodes of evt.target too, in case somebody decides to put nested elements inside of a .mdc-list-item (e.g., an icon).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possibly, but how far down the rabbit hole should we support?

Copy link
Contributor

Choose a reason for hiding this comment

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

I typically walk all the way up the tree until I get to the body or documentElement. It shouldn't really impact performance at all, and it ensures correct behavior in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting we do something like this:

function checkAncestryForClassname(el, classname) {
  if (el.classList.contains(classname)) {
    return true;
  }

  return el.parentNode && checkAncestryForClassname(el.parentNode, classname);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, but it might be better done as a loop to avoid recursive calls? (Also your code would either return true or null as written.)

while (el) {
  if (el.classList.contains(className)) {
    return true;
  }
  el = el.parentNode;
}
return false;

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, what @kfranqueiro said 😀

@@ -174,10 +196,12 @@ export default class MDCSelectFoundation extends MDCFoundation {

let maxTextLength = 0;
for (let i = 0, l = this.adapter_.getNumberOfOptions(); i < l; i++) {
const selectBoxAddedPadding = 26;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this value get pulled out into export const numbers in constants.js?

Does it need to be kept in sync with any SCSS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, given that we're using getComputedStyle in this function already, is it feasible to use that here rather than risk skew between SCSS and JS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You both have good points. Ken, I think that seems feasible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon further review, it's going to be problematic using the adapter method since it returns the value as a string with the unit attached, e.g.: "16px". Going to assign it to a constant and make a note that it has to stay in sync with a scss variable.

@@ -215,7 +216,7 @@ test('#resize falls back to font-{family,size} if shorthand is not supported', (
foundation.resize();
assert.equal(ctx.font, '16px Roboto');
// ceil(letter-spacing * 'longest'.length + longest measured width)
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment, similar to above

align-items: flex-end;
margin-bottom: 6px;
padding-right: 10px;
padding-left: 16px;
Copy link
Contributor

Choose a reason for hiding this comment

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

It this supposed to be indented relative to the __label?

image

It looks a little strange to me, but I'm not sure where the relevant spec is.

@acdvorak acdvorak self-assigned this Aug 9, 2017
Bread, Cereal, Rice, and Pasta
</li>
<li class="mdc-list-item" role="option" id="vegetables" tabindex="-1" aria-disabled="true">
Vegetables (Disabled)
<li class="mdc-list-item" role="option" aria-disabled="true" tabindex="0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing tabindex to 0 here conflicts with the instructions immediately above this example. Which way is correct?

Copy link
Contributor Author

@amsheehan amsheehan Aug 9, 2017

Choose a reason for hiding this comment

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

tabindex="-1" is correct for disabled items.

<div class="mdc-simple-menu mdc-select__menu">
<ul class="mdc-list mdc-simple-menu__items">
<li class="mdc-list-item" role="option" id="grains" tabindex="0">
<li class="mdc-list-item" role="option" aria-disabled="true">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit curious why this was added; isn't this now redundant of the label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure is. Good eye!

<option value="meat">Meat, Poultry, Fish, Dry Beans, Eggs, and Nuts</option>
<option value="fats">Fats, Oils, and Sweets</option>
</select>
<div class="mdc-select">
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to update the wording in the preceding paragraph, since I guess the mdc-select class does not work directly on the native select element, but rather wrapped around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yep. Thanks

| `setAttr(attr: string, value: string) => void` | Sets attribute `attr` to value `value` on the root element. |
| `rmAttr(attr: string) => void` | Removes attribute `attr` from the root element. |
| `computeBoundingRect() => {left: number, top: number}` | Returns an object with a shape similar to a `ClientRect` object, with a `left` and `top` property specifying the element's position on the page relative to the viewport. The easiest way to achieve this is by calling `getBoundingClientRect()` on the root element. |
| `registerPointerDownHandler(evtType: string, handler: EventListener) => void`| Adds an event listener for pointer down interactions to the root element |
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I lack historical context, but what's the reason for increasing the adapter API surface for this? It looks like we could just reuse registerInteractionHandler to register touchstart and pointerdown handlers and likewise for deregistering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mayhap we can, Ken. Mayhap we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can indeed. They are, in fact, the same. :)

@@ -174,10 +196,12 @@ export default class MDCSelectFoundation extends MDCFoundation {

let maxTextLength = 0;
for (let i = 0, l = this.adapter_.getNumberOfOptions(); i < l; i++) {
const selectBoxAddedPadding = 26;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, given that we're using getComputedStyle in this function already, is it feasible to use that here rather than risk skew between SCSS and JS?

setAttr: (attr, value) => this.root_.setAttribute(attr, value),
rmAttr: (attr, value) => this.root_.removeAttribute(attr, value),
computeBoundingRect: () => this.root_.getBoundingClientRect(),
computeBoundingRect: () => this.surface_.getBoundingClientRect(),
registerPointerDownHandler: (evtType, handler) => this.root_.addEventListener(evtType, handler),
Copy link
Contributor

Choose a reason for hiding this comment

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

evtType is inconsistent with type used for existing [de]registerInteractionHandler immediately below. (Also see question I raised in README about why these need to exist.)

},
getComputedStyleValue: (prop) => window.getComputedStyle(this.root_).getPropertyValue(prop),
setStyle: (propertyName, value) => this.root_.style.setProperty(propertyName, value),
getComputedStyleValue: (prop) => window.getComputedStyle(this.surface_).getPropertyValue(prop),
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're changing what element getComputedStyleValue and setStyle operate on, we should update their descriptions in the README which say they operate on the root element.

Also, does changing the target of getComputedStyleValue have any adverse effects on existing resize code, or is that code precisely the point of changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That code is precisely the reason for changing this. The extra DOM node is needed so the visible surface of the select element is different from its container. We need this change because the new styles require overflow: hidden on the visible surface, which would be problematic for showing a menu. We need the new surface however for the new ripple and bottom line behaviors, which are what this method is concerned with

// ceil(letter-spacing * 'longest'.length + longest measured width)
const expectedWidth = Math.ceil((2.5 * 7) + Math.max(...widths));
// ceil(letter-spacing * 'longest'.length + longest measured width + extra padding)
const expectedWidth = Math.ceil((2.5 * 7) + Math.max(...widths) + 26);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we expose 26 as a constant as Andy suggested, could we reuse it here?

component.getDefaultFoundation().adapter_.makeUntabbable();
assert.equal(fixture.tabIndex, -1);
assert.equal(menuEl.tabIndex, -1);
});

test('adapter#getComputedStyleValue gets the computed style value of the prop from the root element', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused; shouldn't this test fail and need to be changed, since getComputedStyleValue was changed to look at the surface rather than root element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was changed. This test now looks at the surface and not the fixture

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm talking about the getComputedStyleValue test, which doesn't show any changes in the diff and still references fixture? I know you changed the setStyle test, but you changed both of these adapter methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I think it's because the surface is set to be the same width as the root. I'll update the test.

@@ -403,12 +425,16 @@ First, wrap both a custom select and a native select within a wrapper element, l
<div class="select-manager">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test that this example still actually works? (See #1025)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'll try it

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this? I don't think the "native element" part of this example even follows the markup we outline above for wrapping native select.

I'm also wondering if we should just cut this example, though, in the long run. cc @lynnjepsen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may need to update the MDC Select example shown here, but what makes you think this wouldn't work anymore?

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

I think I've found some issues during testing:

  • Chrome: Hover ripple doesn't quite cover the entire box; click ripple animates to the wrong place (click and hold to see easily)
  • Firefox: Hover ripple doesn't nearly cover the entire box; click ripple seems OK
  • Edge and IE 11 (ripples disabled): Boxes are wrong size (not as wide as they should be); hover shows darkened circle over box
  • Safari mysteriously seems to be the only browser where things look 100% fine.

</li>
<li class="mdc-list-item" role="option" id="vegetables" aria-disabled="true" tabindex="0">
Vegetables
</li>
<li class="mdc-list-item" role="option" id="fruit" tabindex="0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these (and not Vegetables) removed? More specifically, removing these and leaving a disabled option (with the intent of being a group header) as the last item seems like bad/confusing UX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is not about making a group header. It is about a disabled option. The others were removed for the sake of brevity.

@amsheehan
Copy link
Contributor Author

@kfranqueiro These look like there is a story for the ripple improvements: #194

I'm inclined to leave this as is until that gets played, as this isn't really a bug in mdc-select or mdc-textfield

@kfranqueiro
Copy link
Contributor

RE the ripple on hover... it sounds like #194 is now obsolete according to current design direction, so we should probably just remove hover ripple from this PR (otherwise it'll just make more work to steamroll it later anyway).

package.json Outdated
"autoprefixer": "^7.0.0",
"babel-core": "^6.22.1",
"babel-loader": "^7.0.0",
"autoprefixer": "^7.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think package.json and package-lock.json should be in this PR... oddly, some of the differences change to older versions.


while (el && el !== document.body) {
if (this.adapter_.eventTargetHasClass(el, cssClasses.LIST_ITEM)) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think a return value of true is needed in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True that

@@ -403,12 +425,16 @@ First, wrap both a custom select and a native select within a wrapper element, l
<div class="select-manager">
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this? I don't think the "native element" part of this example even follows the markup we outline above for wrapping native select.

I'm also wondering if we should just cut this example, though, in the long run. cc @lynnjepsen

@@ -451,7 +474,7 @@ custom component, the type will be `MDCSelect:change`, otherwise it will simply
const selectManager = document.querySelector('.select-manager');
const selects = {
custom: MDCSelect.attachTo(selectManager.querySelector('.mdc-select[role="listbox"]')),
native: MDCSelect.attachTo(selectManager.querySelector('select.mdc-select'))
native: MDCSelect.attachTo(selectManager.querySelector('select.mdc-select__surface'))
Copy link
Contributor

Choose a reason for hiding this comment

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

This would not match anything in the above markup; see previous comment.

setAttr: (/* attr: string, value: string */) => {},
rmAttr: (/* attr: string */) => {},
computeBoundingRect: () => /* {left: number, top: number} */ ({left: 0, top: 0}),
registerPointerDownHandler: (/* evtType: string, handler: EventListener */) => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I seem to recall we decided these two APIs were not necessary because they behave identically to [de]registerInteractionHandler? #1097 (comment)

It looks like they're no longer used but they're still here.


// NOTE: For some reasons, stylelint complains that the two patterns below don't follow BEM.
// However, it seems to emit the wrong selector for that pattern. Thus, we disable it above where
// we would normally disable it (&.mdc-textfield--disabled) as a workaround.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is copypasta'd from textfield, along with the stylelint-disable directive below, with no subsequent stylelint-enable directive. Do we even need this directive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh whoops! Yeah, this doesn't need to be this broad, it is needed in the multi select block though. Updated accordingly.

transition: mdc-select-transition(transform);
color: rgba(black, .6);
pointer-events: none;
// Force the label into its own layer to prevent to prevent visible layer promotion adjustments
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "to prevent to prevent"

}
}

&__bottom-line {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see no updates for disabled styles in this PR, and I think we need them...

image

I also see a regression in dark theme (the drop-down arrow is not the correct color, though it seems OK when disabled):

image

'addClass', 'removeClass', 'setAttr', 'rmAttr', 'computeBoundingRect',
'addClass', 'removeClass', 'addClassToLabel', 'removeClassFromLabel', 'addClassToBottomLine',
'removeClassFromBottomLine', 'setBottomLineAttr', 'setAttr', 'rmAttr', 'computeBoundingRect',
'registerPointerDownHandler', 'deregisterPointerDownHandler',
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove [de]registerPointerDownHandler

@@ -214,34 +261,34 @@ test('adapter#focus focuses on the root element', () => {
});

test('adapter#makeTabbable sets the root element\'s tabindex to 0', () => {
const {component, fixture} = setupTest();
fixture.tabIndex = -1;
const {component, menuEl} = setupTest();
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the names of these tests since it no longer affects the root element?

@amsheehan amsheehan force-pushed the feat/select-improvements branch from cd9708f to 28a8c11 Compare October 31, 2017 16:19
@amsheehan amsheehan force-pushed the feat/select-improvements branch from dc88c22 to 689cf23 Compare October 31, 2017 18:56
Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

This branch doesn't seem to build for me. You need to update ripple mixin usage from -base to -surface and from -fg/-bg to -color and -radius.

@amsheehan amsheehan closed this Nov 29, 2017
@amsheehan amsheehan reopened this Nov 29, 2017
Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

For some reason, this is what I see in Edge and IE 11 on the demo page:

image

background-repeat: no-repeat;
background-position: right center;
font-family: Roboto, sans-serif;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this was also removed in a PR that was just merged. Not sure if this will cause a merge conflict next time you merge from master.

@@ -211,8 +313,8 @@
.mdc-list-divider {
margin-left: -16px;
}
// stylelint-enable
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why plugin/selector-bem-pattern was omitted here? We still include it in all other stylelint-enable comments.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

I just noticed that setting disabled doesn't actually disable the select anymore. It makes it look disabled, but you can actually still tab to the select and fully use it. I think this is because we changed the tabbable APIs to reference the menu, but the select itself still has a tabIndex.

Found this while contemplating focus styles, since currently CSS-only selects don't have any distinguishing visual characteristics when focused vs. not (since .mdc-select:focus isn't applicable to the select child of that element).

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

Noticed another potential issue: in LTR, the menu avoids overlapping the arrow in the select, but in RTL, it's still positioned the same way, so it ends up overlapping the arrow on the left side. Can we easily do something about that for RTL, assuming we want the arrow-side visible in both cases?

<div class="mdc-select__menu mdc-simple-menu">
<ul class="mdc-simple-menu__items">
<li class="mdc-list-item" role="option" tabindex="0">An option</li>
<div class="mdc-select__surface">
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you were talking about needing to nest this fixture within another div to make this work, but I don't see an additional parent div from the last commit, I just see added indentation. What happened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read the docs... hahaha

You can operate on the fixture as a whole as if it were any other dom node, so instead of trying to select on mdc-select in the test I can just do fixture.setAttribute('style', 'whatever')

| `makeUntabbable() => void` | Disallows the root element to be tab-focused via keyboard. We achieve this by setting the root element's `tabIndex` property to `-1`. |
| `getComputedStyleValue(propertyName: string) => string` | Get the root element's computed style value of the given dasherized css property `propertyName`. We achieve this via `getComputedStyle(...).getPropertyValue(propertyName). `|
| `setStyle(propertyName: string, value: string) => void` | Sets a dasherized css property `propertyName` to the value `value` on the root element. We achieve this via `root.style.setProperty(propertyName, value)`. |
| `makeTabbable() => void` | Allows the select options to be tab-focused via keyboard. We achieve this by setting the root element's `tabIndex` property to `0`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for changing "select options" back to "root element" on this and the next line, and then these lines end up unchanged, much like the implementation itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmkay

@amsheehan
Copy link
Contributor Author

amsheehan commented Dec 1, 2017

We'd have to do a bit of work to get the position to change in RTL. It would require a new adapter method to detect direction context and some work in the foundation. If that's something we want to do I can add it.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

Upon further investigation/discussion, the menu positioning RTL quirk is probably something that should be fixed in simple-menu logic (it only looks for RTL on its own root element, not any higher). I'll file a separate issue for that sometime soon.

@acdvorak
Copy link
Contributor

acdvorak commented Dec 5, 2017

Can we darken the border color of the CSS-only select on focus? Currently there's no way to tell that the element has focus if there are no items selected.

@amsheehan
Copy link
Contributor Author

@acdvorak Oowee can do!

Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement text field boxes for mdc-select
4 participants