Item in report list is not highlighted and list cannot be navigated with keyboard #37081
Conversation
…e navigated and refactore code
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@mananjadhav |
|
@ZhenjaHorbach Could we have not updated this at one place? I think we could've iterated inside the OptionsSelector? |
That's exactly what I did ) Other changes related only with unnecessary code which we must remove because it is no longer needed |
| @@ -377,6 +377,11 @@ function BaseSelectionList<TItem extends ListItem>( | |||
| isActive: !disableKeyboardShortcuts && isFocused, | |||
| }); | |||
|
|
|||
| const sectionsWithIndexOffset = sections.map((section, index) => { | |||
There was a problem hiding this comment.
Can we move this to a util, as the same code is repeated earlier?
| @@ -92,6 +92,7 @@ function EditReportFieldDropdownPage({fieldName, onSubmit, fieldID, fieldValue, | |||
| textInputLabel={translate('common.search')} | |||
| boldStyle | |||
| sections={sections} | |||
| focusedIndex={0} | |||
There was a problem hiding this comment.
does this have to be set to 0?
There was a problem hiding this comment.
This is exactly the screen where the bug was found
This is needed for searching
So that during the search the first element is selected
There was a problem hiding this comment.
Can you put that in a comment to make this very clear?
There was a problem hiding this comment.
Please explain the why in the comment
There was a problem hiding this comment.
This has already been fixed)
Thanks for review |
|
@mananjadhav |
|
@ZhenjaHorbach I am still testing this but we've got merge conflicts. Can you please resolve them? |
|
@ZhenjaHorbach I am still testing this but we've got merge conflicts now. Also I think the app crashes when I try to do a search on the custom field. Can you please take a look? 
|
Weird |
|
I checked on staging I wasn't able to reproduce. I'll check on main. Meanwhile can you please merge the branch? I think I'll need your support to keep the branch updated. I am taking more time to test as it affects multiple screens. |
I fixed conflicts ) |
|
quick bump @cead22 |
|
Sorry, I was OOO on Friday and we have conflicts again. Please resolve them and I'll review ASAP |
|
Hello ) |
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| const getItemLayout = (_data: OptionsListDataWithIndexOffset[] | null, flatDataArrayIndex: number) => { |
There was a problem hiding this comment.
Is there a reason we need to violation this lint rule, or can we do something like this?
| // eslint-disable-next-line @typescript-eslint/naming-convention | |
| const getItemLayout = (_data: OptionsListDataWithIndexOffset[] | null, flatDataArrayIndex: number) => { | |
| const getItemLayout = (data: OptionsListDataWithIndexOffset[] | null, flatDataArrayIndex: number) => { |
There was a problem hiding this comment.
In this case we get ts error
src/components/OptionsList/types.ts
Outdated
| type OptionsListData = SectionListData<OptionData, Section>; | ||
| type OptionsListDataWithIndexOffset = SectionListData<OptionData, SectionWithIndexOffset>; | ||
|
|
| @@ -92,6 +92,7 @@ function EditReportFieldDropdownPage({fieldName, onSubmit, fieldID, fieldValue, | |||
| textInputLabel={translate('common.search')} | |||
| boldStyle | |||
| sections={sections} | |||
| focusedIndex={0} | |||
There was a problem hiding this comment.
Please explain the why in the comment
I think we need it But inside BaseOptionsList we upgrade our Section for using indexOffset and in this case, we use SectionWithIndexOffset |
But do we need to use it, or can we have a single Section type that we use everywhere? |
I don't think it makes sense to delete this For example |
Can you share your reasoning please? Sorry if it's obvious, but I'm curious why we can't or shouldn't consolidate these types, and use them even on the places outside of BaseList |
Sorry if I didn't give enough details) Since indexOffset is a required parameter inside BaseList So if we use a type with index offset everywhere |
|
Can we make it optional and/or default its value to 0? |
In that case, we will return to this comment ) I think this will make the code more dirty |
|
Alright, sorry for the delay, there's conflicts again. I'll merge after they're fixed |
Hello ) |
|
@cead22 looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
This is not an emergency change, but we're skipping one check and that's making it so we add the label. We're discussing this on slack |
|
✋ 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 staging by https://github.com/cead22 in version: 1.4.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.60-13 🚀
|
Details
Fixed Issues
$ #35843
PROPOSAL: #35843 (comment)
Tests
In addition to EditReportField in this PR, changes were made on other screens related to navigate between elements with keyboard for lists on:
YearPicker
StateSelector
NewChat
BusinessTypeSelector
ReportParticipants
RoomInvite
RoomMembers
Search
WorkspaceSwitcher
RequestStepCurrency
MoneyRequestParticipantsSelector
BaseShareLogList
CountrySelection
TimezoneSelect
TaskAssigneeSelector
WorkspaceInvite
WorkspaceMembers
WorkspaceCategories
WorkspaceMemberDetailsRoleSelection
WorkspaceAutoReportingMonthlyOffset
WorkspaceWorkflowsApprover
Verify that no errors appear in the JS console
Offline tests
In addition to EditReportField in this PR, changes were made on other screens related to navigate between elements with keyboard for lists on:
QA Steps
In addition to EditReportField in this PR, changes were made on other screens related to navigate between elements with keyboard for lists on:
YearPicker
StateSelector
NewChat
BusinessTypeSelector
ReportParticipants
RoomInvite
RoomMembers
Search
WorkspaceSwitcher
RequestStepCurrency
MoneyRequestParticipantsSelector
BaseShareLogList
CountrySelection
TimezoneSelect
TaskAssigneeSelector
WorkspaceInvite
WorkspaceMembers
WorkspaceCategories
WorkspaceMemberDetailsRoleSelection
WorkspaceAutoReportingMonthlyOffset
WorkspaceWorkflowsApprover
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android.mov
iOS: Native
Screen.Recording.2024-02-22.at.15.11.19.mov
iOS: mWeb Safari
Screen.Recording.2024-02-22.at.15.07.03.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
Screen.Recording.2024-02-22.at.14.58.55.mov