Selection List refactor phase 3: base #27767
Conversation
| import OfflineWithFeedback from '../OfflineWithFeedback'; | ||
| import CONST from '../../CONST'; | ||
|
|
||
| function BaseListItem({item, isFocused = false, isDisabled = false, showTooltip, canSelectMultiple, onSelectRow, onDismissError = () => {}}) { |
There was a problem hiding this comment.
Previously the Checkbox (multiple selection) was located in UserListItem, and the Checkmark (single selection) in RadioListItem. Moved them both here, rendering conditionally. Then, the 2 mentioned list items can remain focused on rendering the content, with a better separation of concerns
| const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({ | ||
| maxIndex: flattenedSections.allOptions.length - 1, | ||
| onFocusedIndexChange: scrollToIndex, | ||
| initialFocusedIndex: _.findIndex(flattenedSections.allOptions, (option) => option.keyForList === initiallyFocusedOptionKey), | ||
| disabledIndexes: flattenedSections.disabledOptionsIndexes, | ||
| isActive: !disableKeyboardShortcuts, | ||
| }); |
There was a problem hiding this comment.
Switched from <ArrowKeyFocusManager /> to useArrowKeyFocusManager, no change of behavior
| import {radioListItemPropTypes} from './selectionListPropTypes'; | ||
|
|
||
| function RadioListItem({item, isFocused = false, isDisabled = false, onSelectRow}) { | ||
| function RadioListItem({item, isFocused = false}) { |
There was a problem hiding this comment.
Removed the Checkmark part and moved to BaseListItem
| @@ -47,62 +35,14 @@ function UserListItem({item, isFocused = false, showTooltip, onSelectRow, onDism | |||
| ); | |||
|
|
|||
| return ( | |||
| <OfflineWithFeedback | |||
There was a problem hiding this comment.
Removed the Checkbox part and moved to BaseListItem
src/components/SubscriptAvatar.js
Outdated
| const mainAvatar = ( | ||
| <Avatar | ||
| containerStyles={StyleUtils.getWidthAndHeightStyle(StyleUtils.getAvatarSize(props.size || CONST.AVATAR_SIZE.DEFAULT))} | ||
| source={props.mainAvatar.source} | ||
| size={props.size || CONST.AVATAR_SIZE.DEFAULT} | ||
| name={props.mainAvatar.name} | ||
| type={props.mainAvatar.type} | ||
| /> | ||
| ); | ||
|
|
||
| const secondaryAvatar = ( | ||
| <View | ||
| style={[props.size === CONST.AVATAR_SIZE.SMALL_NORMAL ? styles.flex1 : {}, isSmall ? styles.secondAvatarSubscriptCompact : subscriptStyle]} | ||
| // Hover on overflowed part of icon will not work on Electron if dragArea is true | ||
| // https://stackoverflow.com/questions/56338939/hover-in-css-is-not-working-with-electron | ||
| dataSet={{dragArea: false}} | ||
| > | ||
| <Avatar | ||
| iconAdditionalStyles={[ | ||
| StyleUtils.getAvatarBorderWidth(isSmall ? CONST.AVATAR_SIZE.SMALL_SUBSCRIPT : CONST.AVATAR_SIZE.SUBSCRIPT), | ||
| StyleUtils.getBorderColorStyle(props.backgroundColor), | ||
| ]} | ||
| source={props.secondaryAvatar.source} | ||
| size={isSmall ? CONST.AVATAR_SIZE.SMALL_SUBSCRIPT : CONST.AVATAR_SIZE.SUBSCRIPT} | ||
| fill={themeColors.iconSuccessFill} | ||
| name={props.secondaryAvatar.name} | ||
| type={props.secondaryAvatar.type} | ||
| /> | ||
| </View> | ||
| ); |
There was a problem hiding this comment.
Here I only put the components in variables so we can conditionally render the tooltip and avoid duplicating code
melvin-bot
bot
requested review from
ntdiary
and removed request for
a team
|
✋ 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/cristipaval in version: 1.3.76-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.76-6 🚀
|
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.3.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.77-7 🚀
|
| icons: [ | ||
| { | ||
| source: UserUtils.getAvatar(details.avatar, accountID), | ||
| name: details.login, | ||
| type: CONST.ICON_TYPE_AVATAR, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
We forgot to pass the id of account as separate field in object #29900 (comment)
|
|
||
| // If we don't disable dependencies here, we would need to make sure that the `sections` prop is stable in every usage of this component. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
This change caused a bug - WS - Members - List of users invited to the workspace does not scroll with Up and Down arrows
allOptions is initially empty. But this callback runs only on mount, so it doesn't change when allOptions are populated.
| scrollToIndex(focusedIndex, false); | ||
| firstLayoutRef.current = false; | ||
| }} | ||
| style={[styles.flexGrow0]} |
There was a problem hiding this comment.
This caused minor scrolling issue related to iOS bounce - #30428
| /> | ||
| ); | ||
| }; | ||
|
|
||
| const scrollToFocusedIndexOnFirstRender = useCallback(() => { |
| {item.text} | ||
| </Text> | ||
| <View style={[styles.flex1, styles.alignItemsStart]}> | ||
| <Text style={[styles.optionDisplayName, isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText, item.isSelected && styles.sidebarLinkTextBold]}>{item.text}</Text> |
There was a problem hiding this comment.
We had to specify the numberOfLines={1} to show ellipsis if the text is too long instead of having it overflow (Coming from #32384)
| text={item.text} | ||
| > | ||
| <Text | ||
| style={[styles.optionDisplayName, isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText, styles.sidebarLinkTextBold]} |
There was a problem hiding this comment.
Coming from #28807, it looks like we forgot to test offline for removing a member from a workspace on native app, thus we forgot to add style properly here.
Details
Base PR for the Phase 3 of the Selection List refactor.
Tracker issue: #11795
Child issue: #20354
BaseSelectionList:ArrowKeyFocusManagerwithuseArrowKeyFocusManager(same behavior but hooks)footerContent,disableKeyboardShortcutsandchildrenprops for lists that are gonna need itBaseListItem. This component holds the styles to Checkbox (multiple selection) or Checkmark (single selection). Then, the existingRadioListItemandUserListItemrepresent just the contents of the list item, having a better separation of concernsUserListItem, renameitem.avatartoitem.icons, so we can have multiple icon sources when the list item is a workspaceshowTooltiptoSubscriptAvatar, to render the tooltip conditionallyFixed Issues
$ #20354
PROPOSAL:
Tests
All lists using
SelectionListshould continue working as before. We can check a few of each type (single selection and multiple selection) to make sure both list items are functioning correctly:Offline tests
QA Steps
All lists using
SelectionListshould continue working as before. We can check a few of each type (single selection and multiple selection) to make sure both list items are functioning correctly: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
web.mov
Mobile Web - Chrome
android.web.mov
Mobile Web - Safari
ios.web.mp4
Desktop
desktop.mov
iOS
ios.native.mp4
Android
android.native.mov