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

Handle highlight with mouse events only #331

Merged
merged 2 commits into from
Sep 17, 2020

Conversation

dopatraman
Copy link
Contributor

Goal:
The goal of this PR is to use javascript mouse events to highlight active elements in the emoji dropdown.

Summary:
Right now, the first item of the emoji dropdown stays highlighted even when selecting other elements. The reason for that is that the current logic does not deactivate the first element when the dropdown is first shown. This PR deactivates all elements on every mouse movement.

mouseonly

@dopatraman
Copy link
Contributor Author

pinging @yuku for visibility.

@yuku yuku self-requested a review September 17, 2020 00:33
Copy link
Owner

@yuku yuku left a comment

Choose a reason for hiding this comment

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

LGTM!

@yuku yuku merged commit 686e6b5 into yuku:main Sep 17, 2020
@dopatraman dopatraman deleted the mouse-only-activation branch September 18, 2020 22:51
@aki77
Copy link
Contributor

aki77 commented Oct 9, 2020

This PR has broken the fix for #323.
This is a weird behavior when the dropdown is made scrollable.

If you look at the behavior of GitHub's Mention input, for example, the behavior in v0.1.6 is more natural.

@aki77
Copy link
Contributor

aki77 commented Oct 13, 2020

@yuku @dopatraman

What do you think about the above behavior?

@yuku
Copy link
Owner

yuku commented Oct 14, 2020

This is a weird behavior when the dropdown is made scrollable.

Thanks. I could reproduce the behavior:

0ac6fa7808eee45aaf83f77ae6d5785d

@yuku
Copy link
Owner

yuku commented Oct 14, 2020

Because of my lack of interest on the project, I'd approved this PR by just watching the fancy gif in the PR description.

Now I had closer look at the patch and found some issues in that. (For example, I'd like to avoid using any whenever possible. I should have added a test case to confirm that. Its my bad 😞 )

So I'm going to revert this PR. Sorry for my inconsistency.

yuku added a commit that referenced this pull request Oct 14, 2020
yuku added a commit that referenced this pull request Oct 14, 2020
@aki77
Copy link
Contributor

aki77 commented Oct 14, 2020

Thanks! 🙏

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

Successfully merging this pull request may close these issues.

3 participants