fix(select):change calculateOverlayOffsetY to address offset for small lists #9885

timmoy · 2018-02-11T08:12:02Z

Adds to the _calculateOverlayOffsetY function of select.ts to disable the offset for lists when an option is already selected.
Current implementation determines if the new functionality is enabled based on a new property disableOptionCentering. It is used by adding the text disableOptionCentering to the inside of the <mat-select> tag like you would for multiple.

Example of usage:

<mat-select multiple [(ngModel)]="currentPokemon"
          [required]="pokemonRequired" [disabled]="pokemonDisabled" #pokemonControl="ngModel" disableOptionCentering>
          <mat-option *ngFor="let creature of pokemon" [value]="creature.value">
            {{ creature.viewValue }}
          </mat-option>
        </mat-select>

list with offset:

list without offset:

…mall lists in issue angular#9751

crisbeto

Any new functionality has to have unit tests.

crisbeto · 2018-02-11T08:39:01Z

src/lib/select/select.ts

@@ -1119,6 +1119,11 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    const maxOptionsDisplayed = Math.floor(SELECT_PANEL_MAX_HEIGHT / itemHeight);
    let optionOffsetFromPanelTop: number;

+    // Disable offset for smaller lists by returning 0 as value to offset
+    if (this._getItemCount() <= maxOptionsDisplayed) {


This goes against the Material design spec and I'm expecting it to break a lot of tests once we get to syncing it in. I think it's fine to have a functionality to disable it, but it should be an input that the consumer can opt-out of.

Thanks for the feedback. I've updated the code to use a custom property to disable the offset only if the consumer chooses to do so by specifying it as a custom property. If nothing is specified, then the offset is enabled by default.

timmoy · 2018-02-12T07:37:44Z

I know the travis check is failing (screenshots below), but the errors seems to indicated a lost connection/time out and I'm unsure if this has to do with the code I'm introducing. Any help with the error messages would be much appreciated, thanks!

crisbeto · 2018-02-12T17:26:36Z

src/lib/select/select.ts

@@ -360,6 +360,9 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    this._multiple = coerceBooleanProperty(value);
  }

+  /** Whether the user wants to disable the offset. */
+  @Input () disableOffset: string = '';


Should be a boolean.

crisbeto · 2018-02-12T17:27:04Z

src/lib/select/select.ts

@@ -1119,6 +1122,11 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    const maxOptionsDisplayed = Math.floor(SELECT_PANEL_MAX_HEIGHT / itemHeight);
    let optionOffsetFromPanelTop: number;

+    // Disable offset for smaller lists by returning 0 as value to offset
+    if (this.disableOffset == 'true') {


After changing it to a boolean, this should be if (this.disableOffset).

crisbeto · 2018-02-12T17:28:45Z

src/lib/select/select.ts

@@ -360,6 +360,9 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    this._multiple = coerceBooleanProperty(value);
  }

+  /** Whether the user wants to disable the offset. */


I'm not sure whether people looking at the documentation would know what offset this is referring to. What about calling it something like disableOptionCentering?

I've made the changes you requested for the input:

changed type to boolean

changed name of the variable

changed the if statement check

Note that this comment will end up on the docs site. Something along the lines of "Whether to center the active option over the trigger" would be clearer.

crisbeto · 2018-02-12T17:28:57Z

src/lib/select/select.ts

@@ -1119,6 +1122,11 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    const maxOptionsDisplayed = Math.floor(SELECT_PANEL_MAX_HEIGHT / itemHeight);
    let optionOffsetFromPanelTop: number;

+    // Disable offset for smaller lists by returning 0 as value to offset
+    if (this.disableOffset == 'true') {
+      return 0;


The new option still needs unit tests.

I've added a unit test with option centering disabled along with a component for testing it. I was thinking of adding it to the 'positioning' tests, but didn't do so to make it easier to add other cases.
I've run gulp test after adding it and the headless chrome returns all successful, but I'm not sure if there is a better way to test it.

crisbeto · 2018-02-13T18:24:15Z

src/lib/select/select.spec.ts

+    <div [style.height.px]="heightBelow"></div>
+  `
+})
+class SelectWithoutOptionCentering {


Can you trim this component down only to the functionality necessary to test the new option? Alternatively you should be able to re-use some of the other test components.

I've gone with trimming the component down and got rid of what I could. The @Input and unit test description have also been updated.

crisbeto · 2018-02-13T18:26:01Z

src/lib/select/select.spec.ts

+      formField = fixture.debugElement.query(By.css('mat-form-field')).nativeElement;
+    }));
+
+    it('should align the first option with trigger text if option centering is disabled',


"Pizza" is the second option in this case. Also it may be clearer if it was something like it('should not align the active option with the trigger if centering is disabled').

crisbeto · 2018-02-13T18:27:07Z

src/lib/select/select.ts

@@ -360,6 +360,9 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    this._multiple = coerceBooleanProperty(value);
  }

+  /** Whether the user wants to disable the offset. */


Note that this comment will end up on the docs site. Something along the lines of "Whether to center the active option over the trigger" would be clearer.

…late to trim uneeded parts

willshowell · 2018-02-14T13:55:15Z

src/lib/select/select.ts

@@ -360,6 +360,9 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    this._multiple = coerceBooleanProperty(value);
  }

+  /** Whether to center the active option over the trigger. */
+  @Input () disableOptionCentering: boolean = false;


Can you use coerceBooleanProperty() for this? (see above for examples)

Thanks for the suggestion! I've implemented it and it seems to make it easier for the user to customize now.

willshowell · 2018-02-14T13:56:00Z

src/lib/select/select.ts

@@ -1119,6 +1122,11 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    const maxOptionsDisplayed = Math.floor(SELECT_PANEL_MAX_HEIGHT / itemHeight);
    let optionOffsetFromPanelTop: number;

+    // Disable offset for smaller lists by returning 0 as value to offset


I think this comment is out of date now ("smaller lists")

crisbeto

Good to go once the final set of feedback is addressed.

crisbeto · 2018-02-14T17:10:30Z

src/lib/select/select.ts

@@ -360,6 +360,9 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    this._multiple = coerceBooleanProperty(value);
  }

+  /** Whether to center the active option over the trigger. */
+  @Input () disableOptionCentering: boolean = false;


Remove the extra space before @Input.

I've updated it along with the changes @willshowell mentioned. It changes the way we use the new option so the unit test template has been adjusted accordingly.

…operty pattern

crisbeto

LGTM

angular-automatic-lock-bot · 2019-09-08T09:02:20Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

timmoy added 2 commits February 7, 2018 23:37

fix(select):change calculateOverlayOffsetY to address scrolling for s…

1bf5415

…mall lists in issue angular#9751

fix(select):update implementation of disable offset

c25862a

timmoy requested review from crisbeto and kara as code owners February 11, 2018 08:12

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 11, 2018

crisbeto requested changes Feb 11, 2018

View reviewed changes

timmoy force-pushed the select-list-position branch from 0c8bfb7 to 2846d51 Compare February 12, 2018 06:01

fix(select):update implementation to use component input

d87b1d7

timmoy force-pushed the select-list-position branch from 2846d51 to d87b1d7 Compare February 12, 2018 06:23

crisbeto requested changes Feb 12, 2018

View reviewed changes

fix(select):changed input to boolean, updated name and added unit test

b1d2ada

crisbeto reviewed Feb 13, 2018

View reviewed changes

fix(select):updated comment for input, updated the unit test and temp…

95c0dad

…late to trim uneeded parts

willshowell reviewed Feb 14, 2018

View reviewed changes

crisbeto approved these changes Feb 14, 2018

View reviewed changes

fix(select):updated input spacing, comment and to use coerceBooleanPr…

4b63d1a

…operty pattern

crisbeto reviewed Feb 15, 2018

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Feb 15, 2018

jelbourn merged commit f38bcd9 into angular:master Feb 18, 2018

tinayuangao pushed a commit that referenced this pull request Feb 20, 2018

feat(select): add option to disable option-based alignment (#9885)

76f9c7d

timmoy deleted the select-list-position branch February 21, 2018 16:34

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

fix(select):change calculateOverlayOffsetY to address offset for small lists #9885

fix(select):change calculateOverlayOffsetY to address offset for small lists #9885

Uh oh!

Conversation

timmoy commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timmoy commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Uh oh!

timmoy commented Feb 11, 2018 •

edited

Loading