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

[AutoComplete] set canAutoPosition to false for PopOver #3015

Closed
wants to merge 2 commits into from
Closed

[AutoComplete] set canAutoPosition to false for PopOver #3015

wants to merge 2 commits into from

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Jan 22, 2016

Fixes #2725

@alitaheri
Copy link
Member

@oliviertassinari @newoga Please take a look at this too.

@aweary aweary changed the title set canAutoPosition to false for AutoComplete [AutoComplete] set canAutoPosition to false for PopOver Jan 22, 2016
@alitaheri
Copy link
Member

Although this still doesn't fully address the issue. It makes it better for the time being. I'd be ok for this to be merged. But We should improve it further in the future. @oliviertassinari Do you think it's a breaking change?

@aweary
Copy link
Contributor Author

aweary commented Jan 22, 2016

I'd be completely open to a more robust solution. If someone familiar with the implementation can point me in the right direction, I'd be happy to work on it.

@newoga
Copy link
Contributor

newoga commented Jan 22, 2016

I know very little about the Popover component, unfortunately. 😅 Though from what I've seen, I think the Popover could use some implementation tidying.

Based on #2725, personally I think the current "jumping around" behavior as it relates to AutoComplete is just wrong and I don't think @aweary's change in this PR is a breaking change because the current behavior is broken anyway. I think for AutoComplete, the results should just show below, so if this improves it at all, I think it's fine. This behavior makes a little bit more sense from a DropDownMenu perspective where the currently selected result shouldn't show below but inline.

If we want to fix this particular issue properly, I think we're going to have to start with the Popover though, not the AutoComplete (though AutoComplete could use some improvements too).

@newoga
Copy link
Contributor

newoga commented Jan 22, 2016

Also, I could make an argument it was a breaking change when the AutoComplete switched to using the Popover in the first place because the original behavior was to just show the results below all the time if my memory serves me correctly.

@newoga
Copy link
Contributor

newoga commented Feb 9, 2016

@oliviertassinari I know you were looking at some <AutoComplete /> stuff recently, any thoughts on this?

@mbrookes
Copy link
Member

mbrookes commented Mar 2, 2016

Note sure how a one-line change can have a merge conflict (!), but this looks like a sensible fix until the underlying behaviour is resolved.

@aweary please rebase and we'll merge.

@mbrookes mbrookes assigned mbrookes and unassigned alitaheri Mar 5, 2016
@mbrookes mbrookes added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Mar 5, 2016
@mbrookes
Copy link
Member

mbrookes commented Mar 7, 2016

Ping @aweary 😄

(If you're busy, let me know and I'll fix it up. 👌 )

@aweary
Copy link
Contributor Author

aweary commented Mar 7, 2016

Ah, missed your last ping! I'll get this rebased tonight

@mbrookes
Copy link
Member

mbrookes commented Mar 7, 2016

Thanks! 👍

Merge latest master changes into branch
@aweary
Copy link
Contributor Author

aweary commented Mar 7, 2016

@mbrookes merge conflicts are resolved, though I was having some trouble squashing the commits.

@alitaheri
Copy link
Member

@aweary you will need to git push origin autocomplete-auto-position --force after you git rebase upstream/master -i

@aweary
Copy link
Contributor Author

aweary commented Mar 7, 2016

@alitaheri did that 👍

@alitaheri
Copy link
Member

It's better, but there is another merge commit that needs to be removed. if it proves to be hard. just git reset upstream/mater --hard and reapply your change then force push.

@aweary
Copy link
Contributor Author

aweary commented Mar 7, 2016

@alitaheri I did exactly that and the extra commit didn't go away 😖 I think it's related to the fact that I merged in a bunch of changes to my fork's master and then to this branch.

I can just open a new PR with the change if it matters. Not like its a big change 😛

@alitaheri
Copy link
Member

Yeah that also works 😆 you can also delete this branch and reopen it with the exact same name wherever you want 😁

@aweary
Copy link
Contributor Author

aweary commented Mar 7, 2016

Opened new PR at #3620

@aweary aweary closed this Mar 7, 2016
mbradleyis pushed a commit to staticinstance/material-ui that referenced this pull request Mar 7, 2016
und3fined pushed a commit to und3fined/material-ui that referenced this pull request Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants