Skip to content

Commit

Permalink
fix(select): Fix redundant calculations & allow resyncing foundation …
Browse files Browse the repository at this point in the history 
…to options

- Method `layout` and private method `updateLabel` have been collapsed into one `layout` method. The reasoning is partially to reduce redundancy, but also since there's no reason they should be updated separately. The old layout method may have simply been broken since it doesn't update the label, just the notch, but lack of usage meant this problem didn't surface.
- `layoutOptions` method can now be called by the client whenever the underlying list elements change (i.e. dynamic server fetch), to ensure that the foundation's state is sync'ed up. When typeahead lands in list foundation, this will also allow an optional call to list's layout method to reinitialize typeahead cache. (closes #5646)
- The empty "" option is now properly accounted for as an option. Previously it was ignored, so if you initialized the select with "" having the selected class, it wouldn't be sync'ed to the internal state. This meant that selecting a different option via select.value="blah" wouldn't remove the "selected" classes and attributes on the "" option, meaning it would still act like "" is selected even though selected text shows up as "blah". (closes #5646)
- `foundation.init` is no longer called twice: once by base component.ts and once by select component.ts
- `foundation.init` no longer calls `notchOutline` 3 (???) times (closes #5686)
- `foundation.init` no longer emits change event (closes #5783).

PiperOrigin-RevId: 308892334
  • Loading branch information
@allan-chen
allan-chen authored and Copybara-Service committed Apr 30, 2020
1 parent 0e052b2 commit c801c4b
Show file tree
Hide file tree
Showing 5 changed files with 233 additions and 139 deletions.
8 changes: 7 additions & 1 deletion packages/mdc-select/README.md
View file
Expand Up @@ -447,6 +447,11 @@ Property | Type | Description
`helperTextContent` | `string` (write-only)| Proxies to the foundation's `setHelperTextContent` method when set.
`ripple` | `MDCRipple` | Ripple instance attached to outlined select variant, or `null` for all other variants.

Method Signature | Description
--- | ---
`layout() => void` | Re-calculates if the notched outline should be notched and if the label should float. Proxies to the foundation's `layout()` method.
`layoutOptions() => void` | Synchronizes the list of options with the state of the foundation. Proxies to the foundation's `layoutOptions()` method. Call this whenever menu options are dynamically updated.

### Events

Event Name | Data | Description
Expand Down Expand Up @@ -520,7 +525,8 @@ If you are using a JavaScript framework, such as React or Angular, you can creat
| `setRequired(isRequired: boolean) => void` | Sets the required state through the adapter. |
| `getRequired() => boolean` | Gets the required state through the adapter. |
| `init() => void` | Initializes the foundation. |
| `layout() => void` | Handles determining if the notched outline should be notched. |
| `layout() => void` | Re-calculates if the notched outline should be notched and if the label should float. |
| `layoutOptions() => void` | Synchronizes the list of options with the state of the foundation. Call this whenever menu options are dynamically updated. |
| `setLeadingIconAriaLabel(label: string) => void` | Sets the aria label of the leading icon. |
| `setLeadingIconContent(content: string) => void` | Sets the text content of the leading icon. |
| `setHelperTextContent(content: string) => void` | Sets the content of the helper text. |
Expand Down
12 changes: 10 additions & 2 deletions packages/mdc-select/component.ts
View file
Expand Up @@ -170,7 +170,6 @@ export class MDCSelect extends MDCComponent<MDCSelectFoundation> {
menuSurfaceConstants.strings.OPENED_EVENT, this.handleMenuOpened);
this.menu.listen(
menuConstants.strings.SELECTED_EVENT, this.handleMenuItemAction);
this.foundation_.init();
}

destroy() {
Expand Down Expand Up @@ -276,12 +275,21 @@ export class MDCSelect extends MDCComponent<MDCSelectFoundation> {
}

/**
* Recomputes the outline SVG path for the outline element.
* Re-calculates if the notched outline should be notched and if the label
* should float.
*/
layout() {
this.foundation_.layout();
}

/**
* Synchronizes the list of options with the state of the foundation. Call
* this whenever menu options are dynamically updated.
*/
layoutOptions() {
this.foundation_.layoutOptions();
}

getDefaultFoundation() {
// DO NOT INLINE this variable. For backward compatibility, foundations take a Partial<MDCFooAdapter>.
// To ensure we don't accidentally omit any methods, we need a separate, strongly typed adapter variable.
Expand Down
110 changes: 52 additions & 58 deletions packages/mdc-select/foundation.ts
View file
Expand Up @@ -92,7 +92,7 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> {
// Index of the currently selected menu item.
private selectedIndex: number = numbers.UNSET_INDEX;
// VALUE_ATTR values of the menu items.
private readonly menuItemValues: string[];
private menuItemValues: string[] = [];
// Disabled state
private disabled = false;
// isMenuOpen is used to track the state of the menu by listening to the
Expand All @@ -111,9 +111,6 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> {

this.leadingIcon = foundationMap.leadingIcon;
this.helperText = foundationMap.helperText;

this.menuItemValues = this.adapter_.getMenuItemValues();
this.setDisabled(this.adapter_.hasClass(cssClasses.DISABLED));
}

/** Returns the index of the currently selected menu item, or -1 if none. */
Expand All @@ -126,27 +123,8 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> {
return;
}

const previouslySelectedIndex = this.selectedIndex;
this.selectedIndex = index;

if (this.selectedIndex === numbers.UNSET_INDEX) {
this.adapter_.setSelectedText('');
} else {
this.adapter_.setSelectedText(
this.adapter_.getMenuItemTextAtIndex(this.selectedIndex).trim());
}

if (previouslySelectedIndex !== numbers.UNSET_INDEX) {
this.adapter_.removeClassAtIndex(previouslySelectedIndex, cssClasses.SELECTED_ITEM_CLASS);
this.adapter_.removeAttributeAtIndex(previouslySelectedIndex, strings.ARIA_SELECTED_ATTR);
}
if (this.selectedIndex !== numbers.UNSET_INDEX) {
this.adapter_.addClassAtIndex(
this.selectedIndex, cssClasses.SELECTED_ITEM_CLASS);
this.adapter_.setAttributeAtIndex(
this.selectedIndex, strings.ARIA_SELECTED_ATTR, 'true');
}
this.layout();
this.removeSelectionAtIndex(this.selectedIndex);
this.setSelectionAtIndex(index);

if (closeMenu) {
this.adapter_.closeMenu();
Expand Down Expand Up @@ -199,15 +177,33 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> {
}
}

/**
* Re-calculates if the notched outline should be notched and if the label
* should float.
*/
layout() {
if (this.adapter_.hasLabel()) {
const openNotch = this.getValue().length > 0;
this.notchOutline(openNotch);
const optionHasValue = this.getValue().length > 0;
const isFocused = this.adapter_.hasClass(cssClasses.FOCUSED);
const shouldFloatAndNotch = optionHasValue || isFocused;

this.notchOutline(shouldFloatAndNotch);
this.adapter_.floatLabel(shouldFloatAndNotch);
}
}

/**
* Synchronizes the list of options with the state of the foundation. Call
* this whenever menu options are dynamically updated.
*/
layoutOptions() {
this.menuItemValues = this.adapter_.getMenuItemValues();
const selectedIndex = this.menuItemValues.indexOf(this.getValue());
this.setSelectionAtIndex(selectedIndex);
}

handleMenuOpened() {
if (this.adapter_.getMenuItemValues().length === 0) {
if (this.menuItemValues.length === 0) {
return;
}

Expand All @@ -233,7 +229,7 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> {
* Handles value changes, via change event or programmatic updates.
*/
handleChange() {
this.updateLabel();
this.layout();
this.adapter_.notifyChange(this.getValue());

const isRequired = this.adapter_.hasClass(cssClasses.REQUIRED);
Expand All @@ -254,11 +250,7 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> {
*/
handleFocus() {
this.adapter_.addClass(cssClasses.FOCUSED);

if (this.adapter_.hasLabel()) {
this.notchOutline(true);
this.adapter_.floatLabel(true);
}
this.layout();

this.adapter_.activateBottomLine();
if (this.helperText) {
Expand Down Expand Up @@ -381,37 +373,17 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> {
}
this.adapter_.setMenuWrapFocus(false);

const value = this.getValue();
if (value) {
this.setValue(value);
}

// Initially sync floating label
this.updateLabel();
}

/**
* Notches the outline and floats the label when appropriate.
*/
private updateLabel() {
const value = this.getValue();
const optionHasValue = value.length > 0;

if (this.adapter_.hasLabel()) {
this.notchOutline(optionHasValue);

if (!this.adapter_.hasClass(cssClasses.FOCUSED)) {
this.adapter_.floatLabel(optionHasValue);
}
}
this.setDisabled(this.adapter_.hasClass(cssClasses.DISABLED));
this.layoutOptions();
this.layout();
}

/**
* Unfocuses the select component.
*/
private blur() {
this.adapter_.removeClass(cssClasses.FOCUSED);
this.updateLabel();
this.layout();
this.adapter_.deactivateBottomLine();

const isRequired = this.adapter_.hasClass(cssClasses.REQUIRED);
Expand All @@ -422,6 +394,28 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> {
}
}
}

private setSelectionAtIndex(index: number) {
this.selectedIndex = index;

if (index === numbers.UNSET_INDEX) {
this.adapter_.setSelectedText('');
return;
}

this.adapter_.setSelectedText(
this.adapter_.getMenuItemTextAtIndex(index).trim());
this.adapter_.addClassAtIndex(index, cssClasses.SELECTED_ITEM_CLASS);
this.adapter_.setAttributeAtIndex(
index, strings.ARIA_SELECTED_ATTR, 'true');
}

private removeSelectionAtIndex(index: number) {
if (index !== numbers.UNSET_INDEX) {
this.adapter_.removeClassAtIndex(index, cssClasses.SELECTED_ITEM_CLASS);
this.adapter_.removeAttributeAtIndex(index, strings.ARIA_SELECTED_ATTR);
}
}
}

// tslint:disable-next-line:no-default-export Needed for backward compatibility with MDC Web v0.44.0 and earlier.
Expand Down
93 changes: 71 additions & 22 deletions packages/mdc-select/test/component.test.ts
View file
Expand Up @@ -227,7 +227,7 @@ describe('MDCSelect', () => {
const hasLabel = true;
const {component, menuSurface} =
setupTest(hasOutline, hasLabel, hasMockFoundation, hasMockMenu);
expect(component.selectedIndex).toEqual(-1);
expect(component.selectedIndex).toEqual(0);
component.selectedIndex = 1;
expect(component.selectedIndex).toEqual(1);
menuSurface.parentElement!.removeChild(menuSurface);
Expand All @@ -240,7 +240,7 @@ describe('MDCSelect', () => {
const hasLabel = true;
const {component, menuSurface} =
setupTest(hasOutline, hasLabel, hasMockFoundation, hasMockMenu);
expect(component.selectedIndex).toEqual(-1);
expect(component.selectedIndex).toEqual(0);
component.selectedIndex = 1;
component.selectedIndex = 2;
expect(component.selectedIndex).toEqual(2);
Expand Down Expand Up @@ -309,6 +309,28 @@ describe('MDCSelect', () => {
expect(mockFoundation.setValue).toHaveBeenCalledTimes(1);
});

it('#layout calls foundation.layout', () => {
const hasMockFoundation = true;
const hasMockMenu = true;
const hasOutline = false;
const hasLabel = true;
const {component, mockFoundation} =
setupTest(hasOutline, hasLabel, hasMockFoundation, hasMockMenu);
component.layout();
expect(mockFoundation.layout).toHaveBeenCalled();
});

it('#layoutOptions calls foundation.layoutOptions', () => {
const hasMockFoundation = true;
const hasMockMenu = true;
const hasOutline = false;
const hasLabel = true;
const {component, mockFoundation} =
setupTest(hasOutline, hasLabel, hasMockFoundation, hasMockMenu);
component.layoutOptions();
expect(mockFoundation.layoutOptions).toHaveBeenCalled();
});

it('#get valid forwards to foundation', () => {
const hasMockFoundation = true;
const hasMockMenu = false;
Expand Down Expand Up @@ -408,30 +430,57 @@ describe('MDCSelect', () => {
it('#initialSyncWithDOM sets the selected index if an option has the selected class',
() => {
const fixture = createFixture(`
<div class="mdc-select">
<div class="mdc-select__anchor">
<div class="mdc-select__selected-text"></div>
<label class="mdc-floating-label">Pick a Food Group</label>
<span class="mdc-line-ripple"></span>
</div>
<div class="mdc-select__menu mdc-menu mdc-menu-surface">
<ul class="mdc-list">
<li class="mdc-list-item" data-value=""></li>
<li class="mdc-list-item mdc-list-item--selected" data-value="orange">
Orange
</li>
<li class="mdc-list-item" data-value="apple">
Apple
</li>
</ul>
</div>
</div>
`);
<div class="mdc-select">
<div class="mdc-select__anchor">
<div class="mdc-select__selected-text"></div>
<label class="mdc-floating-label">Pick a Food Group</label>
<span class="mdc-line-ripple"></span>
</div>
<div class="mdc-select__menu mdc-menu mdc-menu-surface">
<ul class="mdc-list">
<li class="mdc-list-item" data-value=""></li>
<li class="mdc-list-item mdc-list-item--selected" data-value="orange">
Orange
</li>
<li class="mdc-list-item" data-value="apple">
Apple
</li>
</ul>
</div>
</div>
`);
const component = new MDCSelect(fixture, /* foundation */ undefined);
expect(component.selectedIndex).toEqual(1);
});

it('#initialSyncWithDOM sets the selected index if empty option has the selected class',
() => {
const fixture = createFixture(`
<div class="mdc-select">
<div class="mdc-select__anchor">
<div class="mdc-select__selected-text"></div>
<label class="mdc-floating-label">Pick a Food Group</label>
<span class="mdc-line-ripple"></span>
</div>
<div class="mdc-select__menu mdc-menu mdc-menu-surface">
<ul class="mdc-list">
<li class="mdc-list-item mdc-list-item--selected" data-value=""></li>
<li class="mdc-list-item" data-value="orange">
Orange
</li>
<li class="mdc-list-item" data-value="apple">
Apple
</li>
</ul>
</div>
</div>
`);
const component = new MDCSelect(fixture, /* foundation */ undefined);
expect(component.selectedIndex).toEqual(0);
});

it('#initialSyncWithDOM disables the select if the disabled class is found',
() => {
const fixture = createFixture(`
Expand Down

0 comments on commit c801c4b

Please sign in to comment.