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: Change emoji navigation with arrow keys #7127

Merged
merged 6 commits into from
Jan 17, 2022

Conversation

mananjadhav
Copy link
Collaborator

@mananjadhav mananjadhav commented Jan 11, 2022

Details

  • Allow the user to go to the first emoji on click of the right arrow
  • Allow the user to go to the search input clicking on the left arrow while first emoji is selected.

Fixed Issues

$ #7035

Tests

  1. Tested ArrowRight to ensure that it selects the first emoji when the cursor is at the end of the string
  2. ArrowDown should ensure that it goes to the first emoji irrespective of the cursor position
  3. ArrowUp and ArrowLeft would select the text in the input and rest of the cursor works fine.
  • Verify that no errors appear in the JS console

QA Steps

  1. Open any chat
  2. Open the emoji picker and type a search term
  3. If the cursor is at the end of the input, and click the right arrow key
  4. It should select the first emoji
  5. When the first emoji is selected, and you click left arrow, it should focus the search input and select the text
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web-emoji-arrow-keys.mov

Mobile Web

NA

Desktop

desktop-emoji-arrow-keys.mov

iOS

NA

Android

NA

@mananjadhav
Copy link
Collaborator Author

@rushatgabhane I've started with this PR. I have a question, can we use onSelectionChange and then set the selection as selection={this.state.selection} for the text input in the following block?

<TextInputFocusable
textAlignVertical="top"
placeholder={this.props.translate('common.search')}
placeholderTextColor={themeColors.textSupporting}
onChangeText={this.filterEmojis}
style={styles.textInput}
defaultValue=""
ref={el => this.searchInput = el}
autoFocus
selectTextOnFocus={this.state.selectTextOnFocus}
/>

I need to be able to set the cursor to the end of the string. At the moment in main as well it goes to the beginning of the string.

@rushatgabhane
Copy link
Member

Yeah sure, go ahead

@mananjadhav
Copy link
Collaborator Author

The selection prop isn't working fine with TextInputFocussable. It automatically resets to {start: 0, end: 0} even if I am setting it {start: 5, end: 5}.

I need this resolved because we want the emoji to be highlighted only if the cursor is at the end of the string. Once this is resolved rest is done.

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 14, 2022

Allow the user to go to the first emoji on click of the right arrow

Just to clarify, we only wanna go to the first emoji when cursor is at the end of search query.

When typing in the emoji search box, when there are no characters in front of the cursor, you click the right arrow key

As mentioned in the issue description

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 14, 2022

The selection prop isn't working fine with TextInputFocussable. It automatically resets to {start: 0, end: 0} even if I am setting it {start: 5, end: 5}.

I think we should skip it then. React Native has an open issue on controlled selection facebook/react-native#29063

Alternative approach

When search is refocused, we should highlight the search query, so it can be replaced by typing.
Let's do it this way. What do you think?

This can be achieved by selectTextOnFocus prop of TextInput

@mananjadhav
Copy link
Collaborator Author

When search is refocused, we should highlight the search query, so it can be replaced by typing.

That is what is handled with ArrowUp that the whole string is selected. This can be done.

@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Jan 14, 2022

That is what is handled with ArrowUp that the whole string is selected. This can be done.

@rushatgabhane Looks like I was wrong. This isn't how ArrowUp and ArrowDown work. Just confirmed on staging.

  1. Irrespective of where my cursor is in the search string, and I click on ArrowDown, it'll go to the first emoji.
  2. Clicking on ArrowUp on the first emoji, takes the cursor to the beginning of the search string

While the second one is easy, I feel we might need the selection prop for the first one to know the cursor position so that either it goes to the next character or to the first emoji. Any other suggestions to handle this?

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 14, 2022

Gothu, but to verify.

  • On ArrowRight, go to first emoji if and only if cursor is at end.
  • On ArrowLeft from first emoji, highlight the text.
  • Change ArrowUp from just focus to highlight the text.

selection prop for the first one to know the cursor position so that either it goes to the next character or to the first emoji. Any other suggestions to handle this?

Umm, I think you can check if cursor is at end using onSelectionChange callback.

@mananjadhav mananjadhav marked this pull request as ready for review January 14, 2022 19:45
@mananjadhav mananjadhav requested a review from a team as a code owner January 14, 2022 19:45
@MelvinBot MelvinBot removed the request for review from a team January 14, 2022 19:45
@mananjadhav
Copy link
Collaborator Author

  • On ArrowRight, go to first emoji if and only if cursor is at end.
  • On ArrowLeft from first emoji, highlight the text.
  • Change ArrowUp from just focus to highlight the text.

@rushatgabhane Yeah the updated PR covers the above cases.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Just left a small comment. Otherwise LGTM and tests well. Great work @mananjadhav!

src/pages/home/report/EmojiPickerMenu/index.js Outdated Show resolved Hide resolved
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

Nice work! Minor suggestions

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

cc: @luacmartins

@luacmartins
Copy link
Contributor

Thanks! @mananjadhav and @parasharrajat!

@luacmartins luacmartins merged commit 8dc6715 into Expensify:main Jan 17, 2022
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @luacmartins in version: 1.1.30-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.31-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

4 participants