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 suggestions not disappering under some conditions #566

Merged
merged 5 commits into from
Sep 10, 2018

Conversation

aberezkin
Copy link
Collaborator

@aberezkin aberezkin commented Sep 7, 2018

In this PR I'm trying to solve the problem I described in this comment. I decided to propose a separate PR because both #417 and #507 seems to be abandoned and stale.

First of all, #380 and #412 are caused by the same code and practically identical, so are solved together in this PR.

There is a thing that nor #417 nor #507 don't take in the account. The thing is that despite we should set this.justSelectedSuggestion to false if the mouse leaves the suggestion we need to set it back to true if mouse enters back because onClick is fired in that case and the suggestion is selected.

The third important thing is that since we are changing onSuggestionMouseDown event, we need to create separate onSuggestionTouchStart event, but because most of the browsers are duplicating touchstart event with mousedown event we need to prevent onSuggestionMouseDown from firing. Usually, you would do e.preventDefault() but as @gaearon pointed out in facebook/react#9809 this won't work on latest chrome with current version of React (but will work in future when facebook/react#2043 is merged) so for now we can just check if this.justSelectedSuggestion is already true to not set this.pressedSuggestion cause it causes some bugs in mobile browsers.

npm run build seems to fail on coverage summary so I also need to write some tests for that (hopefully tests from #417 will suit) but we already can discuss the changes.

Fix #412, fix #380
Close #417, close #507

@moroshko
Copy link
Owner

moroshko commented Sep 9, 2018

@aberezkin Thanks so much for taking the initiative to research and fix this long-standing issue!

Changes look good. Let's just make sure that the tests pass, and if you could add a test for this specific bug that would be appreciated!

@aberezkin
Copy link
Collaborator Author

aberezkin commented Sep 10, 2018

@moroshko, I added some tests and now npm run build seems to be happy.

First 3 tests in when suggestion is dragged describe section are not passing without changes introduced in this PR and last 2 are needed to prevent regression.

@moroshko moroshko merged commit 49cad58 into moroshko:master Sep 10, 2018
@moroshko
Copy link
Owner

@aberezkin Thanks again for the effort!
It's published as v9.4.1 now.

@aberezkin
Copy link
Collaborator Author

@moroshko Nice, thank you.

onSuggestionTouchMove = () => {
this.justSelectedSuggestion = false;
this.pressedSuggestion = null;
this.input.focus();

Choose a reason for hiding this comment

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

Why would you need to refocus the input when onTouchMove?

Copy link
Collaborator Author

@aberezkin aberezkin Mar 28, 2019

Choose a reason for hiding this comment

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

Honestly, I don't remember what exactly happened. But probably touchMove did remove focus from input and that caused suggestions to not disappear on touching something other than input.

Choose a reason for hiding this comment

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

Thanks, I ended up extending the library myself to allow suggestions to remain on screen while dismissing keyboard on an iOS/Android device. So far I haven't had any complications due to rewriting this method. Would you be open to a PR changing this?

Copy link
Collaborator Author

@aberezkin aberezkin Mar 29, 2019

Choose a reason for hiding this comment

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

Of course I will be open to any PR but I am just a contributor. I think it's to better ask @moroshko about this. Anyway there are tests to prevent degradation in this area. Also, I think it would be reasonable to add a prop to control behaviour introduced by your PR, something like endureKeyboardDismiss={false|true}.

Choose a reason for hiding this comment

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

Yes exactly!! Cool I will throw something together this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants