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.

Sign up for GitHub

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

Jump to bottom

Fix Search - Arrow navigation on search stops working #6272

Merged
merged 9 commits into from
Nov 30, 2021
Merged

Fix Search - Arrow navigation on search stops working #6272

merged 9 commits into from
Nov 30, 2021

Conversation

rushatgabhane
Copy link
Member

@rushatgabhane rushatgabhane commented Nov 10, 2021 •
edited
Loading

Details

Comment

Fixed Issues

$ #6131

Tests / QA

  1. Press Control + K
  2. Enter something search
  3. Press the down arrow several times
  4. Verify that you can navigate the search list with the arrow keys

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screencast.from.10-11-21.05-48-24.AM.+03.mp4

Mobile Web

Tested for crash.

Desktop

Screen.Recording.2021-11-10.at.6.55.10.AM.mov

iOS

Tested for crash.

Android

Tested for crash.

@rushatgabhane rushatgabhane requested a review from a team as a code owner November 10, 2021 03:34
@MelvinBot MelvinBot requested review from Julesssss and removed request for a team November 10, 2021 03:34
Julesssss
Julesssss previously approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

Looks good, tests well.

@Julesssss
Copy link
Contributor

Holding, see this post for more info.

@Julesssss Julesssss changed the title Fix Search - Arrow navigation on search stops working [HOLD] Fix Search - Arrow navigation on search stops working Nov 10, 2021
parasharrajat
parasharrajat suggested changes Nov 18, 2021
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

There is one issue here apart from all good. it will remove the keyboard hiding from the Mobile web.
May be you can test that and make it so that it does not break M-web.

src/components/OptionsList/index.native.js Show resolved Hide resolved
@parasharrajat
Copy link
Member

DO NOT MERGE yet. I have a few comments above...Thanks.

@parasharrajat
Copy link
Member

@rushatgabhane Do you mind attaching videos for M-web and native? It affects that behaviour directly so its worth attaching here.

@parasharrajat
Copy link
Member

@rushatgabhane Any update?

@rushatgabhane
Copy link
Member Author

Oops, looks like I missed the notification earlier.
@parasharrajat I'll update you within 2 hours.

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Nov 24, 2021 •
edited
Loading

Ah okay I get it, pull down to dismiss keyboard doesn't work on mWeb.

mWeb.mp4

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Nov 24, 2021 •
edited
Loading

Things work fine on Native even after removing keyboardDismissMode="on-drag" prop.

Android.mp4

@rushatgabhane
Copy link
Member Author

TLDR; While my proposal fixes the issue,
It causes a regression: keyboard won't dismiss on scrolling down for mWeb.

#6272 (comment)

@parasharrajat
Copy link
Member

I am sure we can fix that.

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Nov 24, 2021 •
edited
Loading

Well, we need to use keyboardDismissMode="on-drag" for mWeb only.
Do you think checking for mWeb and conditionally using the prop is a good idea?

We could do that using a regex test for mWeb.
Or just assume smallScreenWidth as mWeb.

What do you say?

@parasharrajat
Copy link
Member

In our app, We consider isSmallScreenWidth to be a Mobile device and thus it can be used for MWeb. I know that a tablet could be Mweb too so I am confused about this as well. Let's take an opinion here @Julesssss Could you please help?

I also think subscribing to Onyx for this component is expensive.

@Julesssss
Copy link
Contributor

I'm tempted to say that we shouldn't care that pull-to-refresh is missing for mWeb. I could be wrong, but I believe that isSmallScreenWidth can be true for small native mobile devices. So if the trade-off is between losing the keyboard dismiss behavior on mWeb or native, I would rather keep it working on native. Does anyone disagree or have a better solution?

@parasharrajat
Copy link
Member

parasharrajat commented Nov 29, 2021 •
edited
Loading

Yeah. If we use isSmallScreenWidth then we can keep the mobile web behavior as well. My main concern here is that It should not create performance issues. OptionList is the most costly component of the app.
@rushatgabhane Could you please test it as well?

@rushatgabhane
Copy link
Member Author

I believe that isSmallScreenWidth can be true for small native mobile devices

@Julesssss I don't think that'd be a problem because isSmallScreenWidth won't be used in index.native.js

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Nov 30, 2021 •
edited
Loading

@parasharrajat
I don't know how to benchmark, or what metrics to look for in this case.

But I couldn't find any glaring performance issues, and I think we're good to merge.
I hope this is what you were looking for.

P.S. Apologies for the forced push, had to remove an unverified commit.

@parasharrajat
Copy link
Member

Thanks I will review shortly

@rushatgabhane
Copy link
Member Author

Also, I doubt withWindowDimensions is using Onyx.
It's just a Higher order component which adds isSmallScreenWidth as a prop.

So I don't think we're adding any substantial overhead.

parasharrajat
parasharrajat suggested changes Nov 30, 2021
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

@rushatgabhane Please merge main into this PR. There have been many important changes recently.

src/components/OptionsList/index.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member

Please remove HOLD from the title as well..

@rushatgabhane rushatgabhane changed the title [HOLD] Fix Search - Arrow navigation on search stops working Fix Search - Arrow navigation on search stops working Nov 30, 2021
rushatgabhane and others added 5 commits November 30, 2021 11:20
@rushatgabhane @parasharrajat
rm unnecessary comment
0f81545 
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
@rushatgabhane @parasharrajat
fix code style
ff2429a 
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
@rushatgabhane
Merge branch 'Expensify:main' into fix-6131
81abddb
@rushatgabhane
fix prop definitions
6704494
@rushatgabhane
import proptypes
eed26e7
parasharrajat
parasharrajat approved these changes Nov 30, 2021
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for the changes.
All yours @Julesssss

Julesssss
Julesssss approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

LGTM

@Julesssss Julesssss merged commit de51e5a into Expensify:main Nov 30, 2021
@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 OSBotify mentioned this pull request Dec 6, 2021
Closed
95 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Dec 7, 2021

🚀 Deployed to staging by @Julesssss in version: 1.1.17-8 🚀

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

@botify botify mentioned this pull request Dec 8, 2021
Closed
@OSBotify
Copy link
Contributor

OSBotify commented Dec 8, 2021

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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
Reviewers

@Julesssss Julesssss Julesssss approved these changes

@parasharrajat parasharrajat parasharrajat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rushatgabhane @Julesssss @parasharrajat @OSBotify