Skip to content

Refocus search input every time a person is selected/unselected#2564

Merged
TomatoToaster merged 8 commits into
mainfrom
amal-new-group-refocus
May 6, 2021
Merged

Refocus search input every time a person is selected/unselected#2564
TomatoToaster merged 8 commits into
mainfrom
amal-new-group-refocus

Conversation

@TomatoToaster

@TomatoToaster TomatoToaster commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

@MonilBhavsar, please review when you get the chance!
CC: @marcaaron, I wasn't sure whether it was fine to reference the ref of a child component like I do here. Wondering if you had ideas about a cleaner approach.

Details

In the Group Chat creator, selecting a user to add will refocus the search bar. This will make it so you don't have to click the search bar again before you search for the next user.

Fixed Issues

Fixes #2452

Tests

Followed the QA steps on a local environment.

QA Steps

  1. Log into E.Cash and try to create a new Group by clicking on the bottom left.
  2. When the side panel opens up, select one of the people or search for someone to add to the group chat.
  3. Verify that each time you add a person by clicking/tapping on them, the serach bar gets refocused and you can search for the next person.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

image

Desktop

image

iOS

image

Android

image

@TomatoToaster TomatoToaster self-assigned this Apr 23, 2021
@TomatoToaster

TomatoToaster commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

Not technically part of this PR but related; I think something is causing this line: https://github.com/Expensify/Expensify.cash/blob/75c554970f89b926e45f26b15acd2ea10da93ef3/src/components/OptionsSelector.js#L89-L91

to not do anything because when the New Group Page is opened, it doesn't actually focus the text input in the beginning.

It's probably because TextInputWithFocusStyles isn't properly passing down the focus function from the forwarded ref?

  • try adding a focus function in there (can you do that with React Native components?)
  • use the autoFocus prop of TextArea and pulling it up to TextInputWithFocusStyles and seeing if it does anything.

@TomatoToaster

Copy link
Copy Markdown
Contributor Author

Ok Ignore that earlier comment. I couldn't figure out a good solution to fix that problem and it isn't relevant to the original issue anyways. It'll be fine to manually click the search bar when the group chat option selector first opens up. This PR still makes it easy to continue adding people.

@TomatoToaster TomatoToaster requested a review from a team May 5, 2021 00:46
@MelvinBot MelvinBot requested review from MonilBhavsar and removed request for a team May 5, 2021 00:46
@TomatoToaster TomatoToaster marked this pull request as ready for review May 5, 2021 00:48
@TomatoToaster TomatoToaster requested a review from a team as a code owner May 5, 2021 00:48
@MelvinBot MelvinBot removed the request for review from a team May 5, 2021 00:48
Comment thread src/pages/NewGroupPage.js Outdated
*/
toggleOption(option) {
// After toggling an option, refocus the search bar to help find the next selection
this.optionSelector.textInput.focus();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While this works, it is pretty unusual and we should try to make this call in the direct parent of that text input.

One option could be to use ref forwarding. But in this case, I'd actually suggest we just move this logic closer to the text input. Here's what I'd suggest:

  1. Create a new method in OptionsSelector called this.selectRow()
  2. In that method, do something like this
selectRow() {
    this.textInput.focus();
    this.props.onSelectRow(...)
}
  1. Update all usages in OptionsSelector to pass this.selectRow instead of this.props.onSelectRow
  2. If you only need this behavior sometimes (since I see we are only adding it to NewGroupPage and not other pages) maybe pass a boolean prop like shouldFocusOnSelectRow to the OptionsSelector ?

@TomatoToaster

Copy link
Copy Markdown
Contributor Author

Alrighty, this is ready for a re-review.

Comment thread src/components/OptionsSelector.js

@MonilBhavsar MonilBhavsar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@TomatoToaster TomatoToaster merged commit d227c30 into main May 6, 2021
@TomatoToaster TomatoToaster deleted the amal-new-group-refocus branch May 6, 2021 15:03
@OSBotify

OSBotify commented May 6, 2021

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging in version: 1.0.38-1🚀

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

@OSBotify

OSBotify commented May 8, 2021

Copy link
Copy Markdown
Contributor

🚀 Deployed to production in version: 1.0.39-5🚀

platform result
🤖 android 🤖 failure ❌
🖥 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.

Group creation - Focus doesn't auto-return to search box after adding a user

4 participants