fix(selection-list): restore focus if active item is destroyed #7125

devversion · 2017-09-16T14:05:58Z

Removes the destroyed and onFocus observables and instead just calls methods on the injected MdSelectionList instance.
Fixes that the active item is not updating if the active item is being destroyed.

crisbeto · 2017-09-16T14:16:09Z

src/lib/list/selection-list.ts

@@ -170,11 +157,11 @@ export class MdListOption extends _MdListOptionMixinBase

  _handleFocus() {
    this._hasFocus = true;
-    this._renderer.addClass(this._element.nativeElement, FOCUSED_STYLE);
+    this.selectionList._setFocusedOption(this);
  }

  _handleBlur() {


Since this is just toggling a boolean, it can also be done inline.

crisbeto · 2017-09-16T14:17:16Z

src/lib/list/selection-list.ts

@@ -334,4 +294,9 @@ export class MdSelectionList extends _MdSelectionListMixinBase
  private _isValidIndex(index: number): boolean {
    return index >= 0 && index < this.options.length;
  }
+
+  /** Returns the index of the specified list option. */
+  private _getIndexFromOption(option: MdListOption): number {


Call this something like _getOptionIndex?

Sounds also good

crisbeto · 2017-09-16T14:26:23Z

src/lib/list/selection-list.ts

+      const optionIndex = this._getIndexFromOption(option);
+
+      // Check whether the option is the last item
+      if (optionIndex - 1 >= 0) {


if (optionIndex > 0) should be a bit more readable.

Yeah that's better. Just took the old logic here.

crisbeto · 2017-09-16T14:28:55Z

src/lib/list/selection-list.ts

+      // Check whether the option is the last item
+      if (optionIndex - 1 >= 0) {
+        this._keyManager.setPreviousItemActive();
+      } else if (optionIndex < this.options.length - 1) {


I don't see any case, aside from optionIndex === 0, where it would get into this block. You can simplify it to check for optionIndex === 0 instead.

Done. Addressed your feedback.

crisbeto

LGTM

crisbeto · 2017-09-17T13:32:02Z

src/lib/list/selection-list.ts


      // Check whether the option is the last item
-      if (optionIndex - 1 >= 0) {
+      if (optionIndex  > 0) {


There's an extra space before the >.

* Removes the `destroyed` and `onFocus` observables and instead just calls methods on the injected `MdSelectionList` instance. * Fixes that the active item is not updating if the active item is being destroyed.

angular-automatic-lock-bot · 2019-09-07T17:02:42Z

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.}

devversion added the pr: needs review label Sep 16, 2017

devversion requested review from crisbeto and jelbourn as code owners September 16, 2017 14:05

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 16, 2017

crisbeto reviewed Sep 16, 2017

View reviewed changes

crisbeto approved these changes Sep 17, 2017

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Sep 17, 2017

devversion force-pushed the fix/selection-list-active-item-destroy branch from 5e5a8ce to b09262f Compare September 17, 2017 14:51

devversion added the P2 The issue is important to a large percentage of users, with a workaround label Sep 18, 2017

devversion force-pushed the fix/selection-list-active-item-destroy branch 3 times, most recently from 8ad9fec to 39d66be Compare September 29, 2017 12:21

fix(selection-list): restore focus if active item is destroyed

1335a0c

* Removes the `destroyed` and `onFocus` observables and instead just calls methods on the injected `MdSelectionList` instance. * Fixes that the active item is not updating if the active item is being destroyed.

devversion force-pushed the fix/selection-list-active-item-destroy branch from 39d66be to 1335a0c Compare September 29, 2017 13:27

andrewseguin merged commit e05f939 into angular:master Sep 29, 2017

devversion deleted the fix/selection-list-active-item-destroy branch November 20, 2017 16:36

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(selection-list): restore focus if active item is destroyed #7125

fix(selection-list): restore focus if active item is destroyed #7125

devversion commented Sep 16, 2017

crisbeto Sep 16, 2017

devversion Sep 16, 2017

crisbeto Sep 16, 2017

devversion Sep 16, 2017

crisbeto Sep 16, 2017

devversion Sep 16, 2017

crisbeto Sep 16, 2017

devversion Sep 17, 2017

crisbeto left a comment

crisbeto Sep 17, 2017

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

fix(selection-list): restore focus if active item is destroyed #7125

fix(selection-list): restore focus if active item is destroyed #7125

Conversation

devversion commented Sep 16, 2017

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

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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