-
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
Standardize arrow navigation in OptionsLists #7702
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
@roryabraham would you mind attempting to resolve the regression again today? Thanks |
@marcaaron interesting.. can you also repro the bug on staging? Because I can't |
nope just tried and it only happens on |
@marcaaron I'm not able to reproduce that on main, which makes me think it's probably related to some API or Auth change that I don't have locally. |
Cool. I will update and try to see if that resolves the issue. Mainly mentioned it here because it broke on a line of code this PR introduced so it seemed like a possible regression. |
🚀 Deployed to production by @roryabraham in version: 1.1.79-17 🚀
|
I believe that this PR introduced this really edge-case bug. We should have either disabled the focus on disabled options or updated the logic in |
@luacmartins what's the bug again, you put the wrong link by mistake i think |
ah sorry, edited with the correct link |
woah, that is an edge case that i would have never thought of. thanks for posting here! |
headerMessage={headerMessage} | ||
hideAdditionalOptionStates | ||
forceTextUnreadStyle | ||
shouldFocusOnSelectRow={this.props.isGroupChat} |
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 causes a minor regression on mobile web, showing the keyboard for every contact selection. Handled it by excluding for mobile browsers
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 was intentional behaviour.
ref={el => this.textInput = el} | ||
value={this.props.value} | ||
label={this.props.textInputLabel} | ||
onChangeText={(text) => { |
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.
Hi, we should have used the onSubmitEditing
prop to enable selection using the software enter key for mobile.
Coming from #21472
// Note: react-native's SectionList automatically strips out any empty sections. | ||
// So we need to reduce the sectionIndex to remove any empty sections in front of the one we're trying to scroll to. | ||
// Otherwise, it will cause an index-out-of-bounds error and crash the app. | ||
let adjustedSectionIndex = sectionIndex; |
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.
SectionList does not appear to be removing the empty sections so this adjustment caused this issue #34112. This has been fixed with removing this adjustment of index.
Details
ArrowKeyFocusManager
component, which takes a few generic props and uses the arrow keys to manage afocusedIndex
state.OptionsSelector
component in preference of theArrowKeyFocusManager
ArrowKeyFocusManager
in theIOUConfirmationList
componentscrollToIndex
correctly for sections listsNotable changes
The IOU currency page used to have an inconsistent UX compared to our other list pages. This PR DRYs that, gives it support for arrow navigation, and makes the UX consistent with other list pages. The behavior before was:
The new behavior is:
Furthermore, it now supports mouseless use via arrow navigation / enter to submit.
Known Limitations
Fixed Issues
$ #7648
PR Reviewer Checklist
main
before submitting the PR### Fixed Issues
section abovesrc/languages/*
files (if applicable)Tests / QA Steps
Log in to New Expensify.
Search Page
All Platforms
x
icon to close the search page.Web/Desktop
CMD+K
to open the search page.Esc
to close the search page.CMD+K
to reopen the search page.ArrowDown
you are taken to the first resultArrowUp
you are taken to the last resultEnter
. The search page should close and that chat should open.Esc
to close the search page.New Chat Page
All Platforms
+
icon on the home screenNew Chat
.Web/Desktop
+
icon on the home screenNew Chat
ESC
– the New Chat Page should close.+
icon on the home screen.New Chat
ArrowDown
you are taken to the first resultArrowUp
you are taken to the last resultEnter
and that chat should open.CMD+Enter
and that chat should open.New Group Page
All Platforms
+
icon on the home screenNew Group
.Create Group
. The New Group page should close and a chat with all the selected users should open.Web/Desktop
+
icon on the home screen.New Group
. The New Group Page should open.ESC
. The New Group page should close.CMD+SHIFT+K
. The New Group Page should open.ESC
. The New Group page should close.New Group
page that the results are scrollable:New Chat Page
in that the search bar is one of the focusable elements.ArrowDown
you are taken to the search bar. None of the elements should be highlighted.ArrowUp
you are taken to the search bar. None of the elements should be highlighted.ArrowUp
, you are taken to the last result.ArrowDown
, you are taken to the first result.Enter
. That result should get checked and move to the top of the list.ArrowDown
key to highlight the first unselected result. It should be completely visible.Enter
to select multiple results.Enter
to de-select that result.CMD+Enter
, and a group should be opened with your selected results.CMD+Enter
, and a group should be opened with your selected results.Enter
(notCMD+Enter
), and a group should be opened with your selected results.IOU Participants (split bill)
All Platforms
+
icon on the home screen.Split Bill
Next
.Web/Desktop
+
icon on the home screen.Split Bill
Next
.ArrowDown
you are taken to the search bar. None of the elements should be highlighted.ArrowUp
you are taken to the search bar. None of the elements should be highlighted.ArrowUp
, you are taken to the last result.ArrowDown
, you are taken to the first result.Enter
. That result should get checked and move to the top of the list.Enter
to select multiple results.Enter
to de-select that result.CMD+Enter
, and you should move to the next page with your selected results.CMD+Enter
, and you should move to the next page with your selected results.Enter
(notCMD+Enter
), and you should move to the next page with your selected results.IOU Participants (1:1 request)
All Platforms
+
icon on the home screen.Request Money
Next
Web/Desktop
+
icon on the home screen.Request Money
Next
ArrowDown
you are taken to the first resultArrowUp
you are taken to the last resultEnter
. That should take you to the next page with your selected user.CMD+Enter
. That should take you to the next page with your new selected user.IOU Confirmation List
All Platforms
+
icon on the home screen.Split Bill
.Next
.Next
.Split YOUR_AMOUNT
+
in the composer and selectSplit Bill
.Split YOUR_AMOUNT
.Web/Desktop
+
in the composer.Split Bill
.Next
.ArrowDown
you are taken to the text input. None of the elements should be highlighted.ArrowUp
you are taken to the text input. None of the elements should be highlighted.ArrowUp
, you are taken to the last result.ArrowDown
, you are taken to the first result.Enter
should select and deselect participants, and their proportional amount should adjust accordingly.CMD + Enter
. Verify that nothing happens (no IOU should be created with just yourself)Enter
. A new request should be created between you and each of your selected participants.+
in the composer.Split Bill
.Next
.CMD+Enter
. A new request should be created between you and each of your selected participants.+
button in the compose bar and clickRequest Money
Next
Enter
. The IOU should be created.Workspace invite page
All Platforms
Settings
->Your Workspace
->Manage Members
->Invite
.Invite
, and verify that you are taken to the previous page and the new invitees are shown as members.Web/Desktop
Settings
->Your Workspace
->Manage Members
->Invite
.ArrowDown
you are taken to the search bar. None of the elements should be highlighted.ArrowUp
you are taken to the search bar. None of the elements should be highlighted.ArrowUp
, you are taken to the last result.ArrowDown
, you are taken to the first result.Enter
. That option should be selected and moved to the top of the list.Enter
. That should deselect the option.Enter
. That should take you to the previous page with the new invitees shown as members.Invite
again.CMD+Enter
. That should take you to the previous page with the new invitees shown as members.IOU currency selection
All Platforms
+
to open the global create menu.$
in US)Web/Desktop
+
to open the global create menu.$
in US)ArrowDown
, you are taken to the top, and vice-versa.Applause (important): Ensure that all of the above steps are covered in our regression test suite in TestFlight
In particular, we may need to add regression tests to cover using arrows to traverse lists. That is the main focus of this PR. Feel free to create an issue if necessary to give yourself more time and make it easier to track. Let me know if you have any questions.
Tested On
Screenshots
Web
Web_Arrow_Navigation.mov
Mobile Web
Desktop
Desktop_Arrow_Nav.mov
iOS
Android