Skip to content

fix(autocomplete): reposition the panel when the amount of options changes #4469

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

Merged

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented May 10, 2017

Fixes the autocomplete panel not being repositioned when the amount of options changes. This is necessary, because adding extra options could push it out of the viewport.

Note: will look into the iOS test failure. It's most likely an issue with the test setup, rather than the new logic.

@crisbeto crisbeto requested a review from kara May 10, 2017 19:19
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 10, 2017
@kara
Copy link
Contributor

kara commented Jun 19, 2017

@crisbeto IIRC, we didn't update the position deliberately because it can be disorienting when the panel switches its position while you're typing. Specifically, if you're moving from more options to fewer options as you type (and the panel can now fit in its first preferred position), the panel moves back to its first position mid-word.

Example case:

  1. Set it up so the autocomplete is at the bottom of the page and empty.
  2. Click to open the panel. It should open above the input.
  3. Begin typing "Alabama".

You can see it switch to the bottom midway through typing, even though the options panel would have fit above.

I definitely agree that the position should update if the panel starts to extend past the viewport (fewer options -> more options). Any way we can make it so the panel only updates position if the current position doesn't fit?

@kara kara assigned crisbeto and unassigned kara Jun 19, 2017
@crisbeto
Copy link
Member Author

We can't right now @kara, but we discussed with Jeremy a while ago that we also shouldn't re-fit the panel in the viewport when you're scrolling. Adding that to the API can be used in this case as well.

@kara
Copy link
Contributor

kara commented Aug 16, 2017

@crisbeto Seems like @jelbourn's in-progress overlay positioning changes will have implications here. Once that gets in, we can re-visit this?

@kara kara removed their request for review August 16, 2017 01:14
@kara kara added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 16, 2017
@crisbeto
Copy link
Member Author

I'm not sure it will @kara. The positioning improvements will change how we manage the overlay's position in general, but I think that we'll still have to recalculate it when the amount of options changes.

@kara
Copy link
Contributor

kara commented Aug 16, 2017

We will probably still need to make changes, but it doesn't seem smart to do so right before major changes to how we handle positioning. Either way, let me know when this PR is again ready for review.

@jelbourn
Copy link
Member

@crisbeto is this worth revisiting once the new positioning is in?

@crisbeto
Copy link
Member Author

Yeah, I think this branch has lived through two iterations of the connected positioning now.

@josephperrott
Copy link
Member

@crisbeto now that the new positioning strategy is in place. Should we rebase and get this in?

@crisbeto crisbeto force-pushed the autocomplete-panel-reposition branch from 60fd3be to 54ab1ab Compare March 31, 2018 08:39
@crisbeto crisbeto requested a review from kara as a code owner March 31, 2018 08:39
@crisbeto crisbeto removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Mar 31, 2018
@crisbeto crisbeto assigned jelbourn and josephperrott and unassigned crisbeto Mar 31, 2018
@crisbeto
Copy link
Member Author

Rebased and fixed some issues with the testing setup @josephperrott.

@josephperrott
Copy link
Member

It looks like the new test failed on travis?

…anges

Fixes the autocomplete panel not being repositioned when the amount of options changes. This is necessary, because adding extra options could push it out of the viewport.
@crisbeto crisbeto force-pushed the autocomplete-panel-reposition branch from 54ab1ab to 497602d Compare April 2, 2018 16:28
@crisbeto
Copy link
Member Author

crisbeto commented Apr 2, 2018

It's fixed now @josephperrott.

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@josephperrott josephperrott added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jun 26, 2018
@josephperrott josephperrott merged commit 2b80dbf into angular:master Jun 29, 2018
@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 9, 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.

6 participants