Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled #13774

Merged

Conversation

crisbeto
Copy link
Member

Fixes the first option not appearing as highlighted if the consumer has set floatLabel="never" and autoActiveFirstOption. It looks like we were depending on the transitioned event from the label to kick off the change detection that shows the highlight.

Fixes #13734.

…y if the floating label is disabled

Fixes the first option not appearing as highlighted if the consumer has set `floatLabel="never"` and `autoActiveFirstOption`. It looks like we were depending on the `transitioned` event from the label to kick off the change detection that shows the highlight.

Fixes angular#13734.
@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Oct 23, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 23, 2018
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -493,6 +493,7 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
switchMap(() => {
this._resetActiveItem();
this.autocomplete._setVisibility();
this._changeDetectorRef.detectChanges();
Copy link
Member

Choose a reason for hiding this comment

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

I see that it's already set up this way, but I just noticed that it's weird for this switchMap to have side effects. No action necessary here, but something to keep in mind.

Copy link
Contributor

@thomaspink thomaspink Oct 29, 2018

Choose a reason for hiding this comment

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

@crisbeto @jelbourn As we only need the detectChanges when firstStable fires (optionChanges does not need it, because it is resulting out of an user action) we could limit it to only the firstStable event. Maybe like so:

return merge(firstStable, optionChanges)
  .pipe(
    // create a new stream of panelClosingActions, replacing any previous streams
    // that were created, and flatten it so our stream only emits closing events...
    switchMap((optionChange) => {
      this._resetActiveItem();
      this.autocomplete._setVisibility();

      if (this.panelOpen) {
        this._overlayRef!.updatePosition();
      }

      // Only detectChanges when firstStable fires
      if (!optionChange) {
        this._changeDetectorRef.detectChanges();
      }

      return this.panelClosingActions;
    }),
    // ...

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 26, 2018
@ngbot
Copy link

ngbot bot commented Oct 26, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "continuous-integration/travis-ci/pr" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mmalerba mmalerba merged commit c99c512 into angular:master Dec 4, 2018
mmalerba added a commit that referenced this pull request Dec 5, 2018
…correctly if the floating label is disabled (#13774)"

This reverts commit c99c512.
mmalerba added a commit that referenced this pull request Dec 5, 2018
…correctly if the floating label is disabled" (#14396)

* Revert "fix(drag-drop): error on touch end (#14392)"

This reverts commit 53cecbb.

* Revert "fix(menu): reduce specificity of icon selector (#14389)"

This reverts commit 74e945a.

* Revert "fix(drag-drop): throw better error when attaching to non-element node (#14221)"

This reverts commit 31f0e6d.

* Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled (#13774)"

This reverts commit c99c512.
@mmalerba
Copy link
Contributor

mmalerba commented Dec 5, 2018

@crisbeto this somehow broke some tests in google so I had to revert

mmalerba added a commit that referenced this pull request Dec 5, 2018
…correctly if the floating label is disabled (#13774)"

This reverts commit c99c512.
mmalerba added a commit that referenced this pull request Dec 5, 2018
… in trigger" (#14398)

* Revert "Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled" (#14396)"

This reverts commit aa17fbd.

* Revert "fix(drag-drop): error on touch end (#14392)"

This reverts commit 53cecbb.

* Revert "fix(menu): reduce specificity of icon selector (#14389)"

This reverts commit 74e945a.

* Revert "fix(drag-drop): throw better error when attaching to non-element node (#14221)"

This reverts commit 31f0e6d.

* Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled (#13774)"

This reverts commit c99c512.

* Revert "fix(autocomplete): update template when changing autocomplete in trigger (#13814)"

This reverts commit 904a5ea.
mmalerba pushed a commit that referenced this pull request Dec 10, 2018
…y if the floating label is disabled (#13774)

Fixes the first option not appearing as highlighted if the consumer has set `floatLabel="never"` and `autoActiveFirstOption`. It looks like we were depending on the `transitioned` event from the label to kick off the change detection that shows the highlight.

Fixes #13734.
mmalerba added a commit that referenced this pull request Dec 10, 2018
…correctly if the floating label is disabled" (#14396)

* Revert "fix(drag-drop): error on touch end (#14392)"

This reverts commit 53cecbb.

* Revert "fix(menu): reduce specificity of icon selector (#14389)"

This reverts commit 74e945a.

* Revert "fix(drag-drop): throw better error when attaching to non-element node (#14221)"

This reverts commit 31f0e6d.

* Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled (#13774)"

This reverts commit c99c512.
mmalerba added a commit that referenced this pull request Dec 10, 2018
… in trigger" (#14398)

* Revert "Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled" (#14396)"

This reverts commit aa17fbd.

* Revert "fix(drag-drop): error on touch end (#14392)"

This reverts commit 53cecbb.

* Revert "fix(menu): reduce specificity of icon selector (#14389)"

This reverts commit 74e945a.

* Revert "fix(drag-drop): throw better error when attaching to non-element node (#14221)"

This reverts commit 31f0e6d.

* Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled (#13774)"

This reverts commit c99c512.

* Revert "fix(autocomplete): update template when changing autocomplete in trigger (#13814)"

This reverts commit 904a5ea.
josephperrott pushed a commit to josephperrott/components that referenced this pull request Jan 14, 2019
…y if the floating label is disabled (angular#13774)

Fixes the first option not appearing as highlighted if the consumer has set `floatLabel="never"` and `autoActiveFirstOption`. It looks like we were depending on the `transitioned` event from the label to kick off the change detection that shows the highlight.

Fixes angular#13734.
josephperrott pushed a commit to josephperrott/components that referenced this pull request Jan 14, 2019
…correctly if the floating label is disabled" (angular#14396)

* Revert "fix(drag-drop): error on touch end (angular#14392)"

This reverts commit 53cecbb.

* Revert "fix(menu): reduce specificity of icon selector (angular#14389)"

This reverts commit 74e945a.

* Revert "fix(drag-drop): throw better error when attaching to non-element node (angular#14221)"

This reverts commit 31f0e6d.

* Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled (angular#13774)"

This reverts commit c99c512.
josephperrott pushed a commit to josephperrott/components that referenced this pull request Jan 14, 2019
… in trigger" (angular#14398)

* Revert "Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled" (angular#14396)"

This reverts commit aa17fbd.

* Revert "fix(drag-drop): error on touch end (angular#14392)"

This reverts commit 53cecbb.

* Revert "fix(menu): reduce specificity of icon selector (angular#14389)"

This reverts commit 74e945a.

* Revert "fix(drag-drop): throw better error when attaching to non-element node (angular#14221)"

This reverts commit 31f0e6d.

* Revert "fix(autocomplete): auto-highlighted first option not display correctly if the floating label is disabled (angular#13774)"

This reverts commit c99c512.

* Revert "fix(autocomplete): update template when changing autocomplete in trigger (angular#13814)"

This reverts commit 904a5ea.
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete]: autoActiveFirstOption not working when tabbing in with disabled floatLabel
5 participants