refactor: SelectionList multiple selection #22622
Conversation
thiagobrez
changed the title
|
Bit of feedback here – to merge a PR of this size and scope without any test or QA steps listed in the PR description is frankly unacceptable. Can we please add a thorough list of QA steps for Applause to follow to QA this on staging? |
|
Also, this PR is associated with a regression on staging |
Thanks for the feedback @roryabraham! There are tests added to the PR description I do agree that those tests could be improved, but I also think we should first figure out how we could know the product specs better, what is that single source of truth based on which we know the expectations and lay them down thoroughly in a list of tests. In this specific case, the tests mentioned the features that were impacted by the PR and we expected the QA team to run all test cases that they have for those features. |
|
Great response @cristipaval! To answer your questions/concerns:
|
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.3.58-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.57-6 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.58-5 🚀
|
|
Bug zero: This PR caused this regression: #25859 
Taken from: #25859 (comment) |
| const selectRow = (item, index) => { | ||
| // In single-selection lists we don't care about updating the focused index, because the list is closed after selecting an item | ||
| if (canSelectMultiple) { | ||
| if (sections.length === 1) { | ||
| // If the list has only 1 section (e.g. Workspace Members list), we always focus the next available item | ||
| const nextAvailableIndex = _.findIndex(flattenedSections.allOptions, (option, i) => i > index && !option.isDisabled); | ||
| setFocusedIndex(nextAvailableIndex); | ||
| } else { | ||
| // If the list has multiple sections (e.g. Workspace Invite list), we focus the first one after all the selected (selected items are always at the top) | ||
| const selectedOptionsCount = item.isSelected ? flattenedSections.selectedOptions.length - 1 : flattenedSections.selectedOptions.length + 1; | ||
| setFocusedIndex(selectedOptionsCount); | ||
| } | ||
| } | ||
|
|
||
| onSelectRow(item); | ||
| }; | ||
|
|
||
| const selectFocusedOption = () => { | ||
| const focusedOption = flattenedSections.allOptions[focusedIndex]; | ||
|
|
||
| if (!focusedOption || focusedOption.isDisabled) { | ||
| return; | ||
| } | ||
|
|
||
| selectRow(focusedOption, focusedIndex); | ||
| }; |
There was a problem hiding this comment.
| </View> | ||
| </PressableWithFeedback> | ||
| )} | ||
| <SectionList |
There was a problem hiding this comment.
Coming from #26243:
This refactor caused minor inconsistency with OptionsList.
stickySectionHeadersEnabled prop is enabled by default in SectionList.
OptionsList disabled it but SelectionList from this PR didn't.
| keyboardType={keyboardType} | ||
| selectTextOnFocus | ||
| spellCheck={false} | ||
| onSubmitEditing={selectFocusedOption} |
There was a problem hiding this comment.
blurOnSubmit prop could be passed here to be consistent with BaseOptionsSelector behavior.
Issue: #28072
| result.push({ | ||
| keyForList: accountID, | ||
| isSelected: _.contains(selectedEmployees, Number(accountID)), | ||
| isDisabled: accountID === props.session.accountID || details.login === props.policy.owner || policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, |
There was a problem hiding this comment.
accountID should have been converted to a Number before matching. This caused #28539
|
This PR caused conflicting listeners with ConfirmModal, which caused #33201. |
| result.push({ | ||
| keyForList: accountID, | ||
| isSelected: _.contains(selectedEmployees, Number(accountID)), | ||
| isDisabled: accountID === props.session.accountID || details.login === props.policy.owner || policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, |
There was a problem hiding this comment.
I think the isDisabled check contributed to creating the following issue:
but since the transition from optimistic local data to real BE returned data wasn't tested when this PR was merged -> it wasn't reported as an issue until recently.
Details
Phase 2 of 3, regarding selection lists refactor. Tracker issue: #11795
This PR iterates on Phase 1 by extending
SelectionListRadioto also be able to handle multiple selection lists, since both would share all the basics such as keyboard listeners, safe area, layout calculation, focus and scrolling.Following pages were changed:
WorkspaceMembersPageWorkspaceInvitePageThe tracker issue also outlined pages to create a new group and split money, but it was decided that it's best if we don't touch those for now, as things are still getting figured out.
SelectionListRadioand all related files were renamed toSelectionListSelectionListnow accepts the following new props:canSelectMultiple: changes the rendered item to be the newly createdCheckboxListItemonSelectAll: callback to fire when the "Select All" checkbox is pressedonDismissError: callback to fire when an error is dismissedSelectionListnow renders a section header if provided insection.titleCreated a
formatMemberForListinOptionsListUtilsto format apersonalDetailsoruserToInviteto the correct format accepted bySelectionList. Previously, those gigantic objects were being passed down to the list and only a few properties were being used, this way it's clear what properties an item needs to haveOptionsListUtils.getMemberInviteOptionsdirectly because it's a big logic and still being used by the lists that are not refactored yetAdded new Storybook stories for the new list variations
Added new Reassure performance tests for the new list variations, and also a case for scrolling the list
Fixed Issues
$ #20353
Tests
Offline tests
QA Steps
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)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web - Chrome
web.chrome.mov
Web - Safari
web.safari.mov
Mobile Web - Chrome
android.web.mov
Mobile Web - Safari
ios.web.mp4
Desktop
desktop.mov
iOS
ios.native.mp4
Android
android.native.mov