-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: picker an arrow navigation #3871
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, but a few other feature notes:
- When search is refocused, we should highlight the string so that the old search will be replaced by anything new you type.
- When you start typing again (any key other than arrow or enter), then the search should refocus.
This one is more of a nice-to-have, but if you:
- Hover over an emoji to highlight it.
- Use the down-arrow to focus the emoji,
- Then the focus will go to the one that you have hovered, which feels a bit weird to me. I think it should just highlight the first emoji in the first row.
@@ -110,7 +105,9 @@ class EmojiPickerMenu extends Component { | |||
this.props.onEmojiSelected(this.state.filteredEmojis[this.state.highlightedIndex].code); | |||
} | |||
}; | |||
document.addEventListener('keydown', this.keyDownHandler); | |||
|
|||
// Keyboard events are not bubbling in RN-Web |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment isn't super helpful in it's current state – if you include it I think it's better to explain why bubbling in this case is necessary for this feature to work.
@jasperhuangg Any input for this as you originally created the Picker. |
I believe @stitesExpensify originally created the emoji picker. That last item I feel less strongly about too, so if you disagree let's just start with the first two. |
My bad ... didn't mean to close this at all. |
@roryabraham 1, 2 is implemented and PR is updated. Thanks. |
I think @stitesExpensify originally implemented the UI for the picker, but anything involving arrow key presses/focusing/events was added on later by me. So I actually set a state variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and tests well. Just want to clean up the comment one more time before we merge.
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
Updated @roryabraham. |
@roryabraham Gentle bump. Thanks |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production in version: 1.0.77-5🚀
|
Details
Fixed Issues
$ Fixes #3858
Tests | QA Steps
Tested On
Screenshots
Web
emojipicker.mp4
Mobile Web
Desktop
iOS
Android