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.

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

DMs to show all avatars #21402

Merged
merged 16 commits into from
Jul 5, 2023
Merged
4 changes: 4 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -1109,6 +1109,10 @@ const CONST = {
HEADER: 'header',
MENTION_ICON: 'mention-icon',
},
AVATAR_ROW_SIZE: {
DEFAULT: 4,
LARGE_SCREEN: 8,
},
OPTION_MODE: {
COMPACT: 'compact',
DEFAULT: 'default',
Expand Down
232 changes: 139 additions & 93 deletions src/components/MultipleAvatars.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {memo} from 'react';
import React, {memo, useEffect, useState} from 'react';
import PropTypes from 'prop-types';
import {View} from 'react-native';
import _ from 'underscore';
Expand Down Expand Up @@ -32,6 +32,9 @@ const propTypes = {
/** Prop to identify if we should load avatars vertically instead of diagonally */
shouldStackHorizontally: PropTypes.bool,

/** Prop to identify if we should display avatars in rows */
shouldDisplayAvatarsInRows: PropTypes.bool,

/** Whether the avatars are hovered */
isHovered: PropTypes.bool,

Expand All @@ -49,6 +52,9 @@ const propTypes = {

/** Whether avatars are displayed with the highlighted background color instead of the app background color. This is primarily the case for IOU previews. */
shouldUseCardBackground: PropTypes.bool,

/** Prop to limit the amount of avatars displayed horizontally */
maxAvatarsInRow: PropTypes.number,
};

const defaultProps = {
Expand All @@ -57,20 +63,48 @@ const defaultProps = {
secondAvatarStyle: [StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)],
fallbackIcon: undefined,
shouldStackHorizontally: false,
shouldDisplayAvatarsInRows: false,
isHovered: false,
isPressed: false,
isFocusMode: false,
isInReportAction: false,
shouldShowTooltip: true,
shouldUseCardBackground: false,
maxAvatarsInRow: CONST.AVATAR_ROW_SIZE.DEFAULT,
};

function MultipleAvatars(props) {
const [avatarRows, setAvatarRows] = useState([props.icons]);
let avatarContainerStyles = props.size === CONST.AVATAR_SIZE.SMALL ? [styles.emptyAvatarSmall, styles.emptyAvatarMarginSmall] : [styles.emptyAvatar, styles.emptyAvatarMargin];
const singleAvatarStyles = props.size === CONST.AVATAR_SIZE.SMALL ? styles.singleAvatarSmall : styles.singleAvatar;
const secondAvatarStyles = [props.size === CONST.AVATAR_SIZE.SMALL ? styles.secondAvatarSmall : styles.secondAvatar, ...props.secondAvatarStyle];
const tooltipTexts = props.shouldShowTooltip ? _.pluck(props.icons, 'name') : [''];

const calculateAvatarRows = () => {
// If we're not displaying avatars in rows or the number of icons is less than or equal to the max avatars in a row, return a single row
if (!props.shouldDisplayAvatarsInRows || props.icons.length <= props.maxAvatarsInRow) {
setAvatarRows([props.icons]);
return;
}

// Calculate the size of each row
const rowSize = Math.min(Math.ceil(props.icons.length / 2), props.maxAvatarsInRow);

// Slice the icons array into two rows
const firstRow = props.icons.slice(rowSize);
const secondRow = props.icons.slice(0, rowSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 Coming from #31151

The slice seems backward between firstRow and secondRow, so causing the issue.


// Update the state with the two rows as an array
setAvatarRows([firstRow, secondRow]);
};

useEffect(() => {
calculateAvatarRows();

// The only dependencies of the effect are based on props, so we can safely disable the exhaustive-deps rule
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.icons, props.maxAvatarsInRow, props.shouldDisplayAvatarsInRows]);
Comment on lines +101 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

Greetings,

This code has introduced a regression in #24062. The useEffect is triggered after the re-rendering process, during which the app uses outdated state data along with the new prop data, leading to a crash. Rather than using useEffect in combination with State, the recommended approach involves using useMemo to recalculate the data upon prop changes. This adjustment guarantees the correct flow of data.


if (!props.icons.length) {
return null;
}
Expand Down Expand Up @@ -118,114 +152,126 @@ function MultipleAvatars(props) {
}

return (
<View style={avatarContainerStyles}>
<>
{props.shouldStackHorizontally ? (
<>
{_.map([...props.icons].splice(0, 4), (icon, index) => (
<UserDetailsTooltip
key={`stackedAvatars-${index}`}
accountID={icon.id}
>
<View
style={[
styles.justifyContentCenter,
styles.alignItemsCenter,
StyleUtils.getHorizontalStackedAvatarBorderStyle({
isHovered: props.isHovered,
isPressed: props.isPressed,
isInReportAction: props.isInReportAction,
shouldUseCardBackground: props.shouldUseCardBackground,
}),
StyleUtils.getHorizontalStackedAvatarStyle(index, overlapSize, oneAvatarBorderWidth, oneAvatarSize.width),
icon.type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(props.size, icon.type) : {},
]}
>
<Avatar
source={icon.source || props.fallbackIcon}
fill={themeColors.iconSuccessFill}
size={props.size}
name={icon.name}
type={icon.type}
/>
</View>
</UserDetailsTooltip>
))}
{props.icons.length > 4 && (
<Tooltip
// We only want to cap tooltips to only the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
text={tooltipTexts.slice(3, 10).join(', ')}
>
<View
style={[
styles.alignItemsCenter,
styles.justifyContentCenter,
StyleUtils.getHorizontalStackedAvatarBorderStyle({
isHovered: props.isHovered,
isPressed: props.isPressed,
isInReportAction: props.isInReportAction,
shouldUseCardBackground: props.shouldUseCardBackground,
}),

// Set overlay background color with RGBA value so that the text will not inherit opacity
StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.overlay, variables.overlayOpacity),
StyleUtils.getHorizontalStackedOverlayAvatarStyle(oneAvatarSize, oneAvatarBorderWidth),
props.icons[3].type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(props.size, props.icons[3].type) : {},
]}
_.map(avatarRows, (avatars, rowIndex) => (
<View
style={avatarContainerStyles}
key={`avatarRow-${rowIndex}`}
>
{_.map([...avatars].splice(0, props.maxAvatarsInRow), (icon, index) => (
<UserDetailsTooltip
key={`stackedAvatars-${index}`}
accountID={icon.id}
>
<View
style={[styles.justifyContentCenter, styles.alignItemsCenter, StyleUtils.getHeight(oneAvatarSize.height), StyleUtils.getWidthStyle(oneAvatarSize.width)]}
style={[
styles.justifyContentCenter,
styles.alignItemsCenter,
StyleUtils.getHorizontalStackedAvatarBorderStyle({
isHovered: props.isHovered,
isPressed: props.isPressed,
isInReportAction: props.isInReportAction,
shouldUseCardBackground: props.shouldUseCardBackground,
}),
StyleUtils.getHorizontalStackedAvatarStyle(index, overlapSize, oneAvatarBorderWidth, oneAvatarSize.width),
icon.type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(props.size, icon.type) : {},
]}
>
<Text style={[styles.avatarInnerTextSmall, StyleUtils.getAvatarExtraFontSizeStyle(props.size)]}>{`+${props.icons.length - 4}`}</Text>
</View>
</View>
</Tooltip>
)}
</>
) : (
<View style={singleAvatarStyles}>
<UserDetailsTooltip accountID={props.icons[0].id}>
{/* View is necessary for tooltip to show for multiple avatars in LHN */}
<View>
<Avatar
source={props.icons[0].source || props.fallbackIcon}
fill={themeColors.iconSuccessFill}
size={props.isFocusMode ? CONST.AVATAR_SIZE.MID_SUBSCRIPT : CONST.AVATAR_SIZE.SMALLER}
imageStyles={[singleAvatarStyles]}
name={props.icons[0].name}
type={props.icons[0].type}
/>
</View>
</UserDetailsTooltip>
<View style={secondAvatarStyles}>
{props.icons.length === 2 ? (
<UserDetailsTooltip accountID={props.icons[1].id}>
<View>
<Avatar
source={props.icons[1].source || props.fallbackIcon}
source={icon.source || props.fallbackIcon}
fill={themeColors.iconSuccessFill}
size={props.isFocusMode ? CONST.AVATAR_SIZE.MID_SUBSCRIPT : CONST.AVATAR_SIZE.SMALLER}
imageStyles={[singleAvatarStyles]}
name={props.icons[1].name}
type={props.icons[1].type}
size={props.size}
name={icon.name}
type={icon.type}
/>
</View>
</UserDetailsTooltip>
) : (
<Tooltip text={tooltipTexts.slice(1).join(', ')}>
<View style={[singleAvatarStyles, styles.alignItemsCenter, styles.justifyContentCenter]}>
<Text
selectable={false}
style={props.size === CONST.AVATAR_SIZE.SMALL ? styles.avatarInnerTextSmall : styles.avatarInnerText}
))}
{avatars.length > props.maxAvatarsInRow && (
<Tooltip
// We only want to cap tooltips to only the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
text={tooltipTexts.slice(3, 10).join(', ')}
Copy link
Contributor

Choose a reason for hiding this comment

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

It was missed to change tooltip text while implementing this feature so it caused regression - #22951
More details:
Bug: #22951 (comment)
Root cause: #22951 (comment)

>
<View
style={[
styles.alignItemsCenter,
styles.justifyContentCenter,
StyleUtils.getHorizontalStackedAvatarBorderStyle({
isHovered: props.isHovered,
isPressed: props.isPressed,
isInReportAction: props.isInReportAction,
shouldUseCardBackground: props.shouldUseCardBackground,
}),

// Set overlay background color with RGBA value so that the text will not inherit opacity
StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.overlay, variables.overlayOpacity),
StyleUtils.getHorizontalStackedOverlayAvatarStyle(oneAvatarSize, oneAvatarBorderWidth),
props.icons[3].type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(props.size, props.icons[3].type) : {},
]}
>
<View
style={[
styles.justifyContentCenter,
styles.alignItemsCenter,
StyleUtils.getHeight(oneAvatarSize.height),
StyleUtils.getWidthStyle(oneAvatarSize.width),
]}
>
{`+${props.icons.length - 1}`}
</Text>
<Text style={[styles.avatarInnerTextSmall, StyleUtils.getAvatarExtraFontSizeStyle(props.size)]}>{`+${avatars.length - props.maxAvatarsInRow}`}</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

This Text should be non-selectable and caused regression here. We updated and make this non-selectable here.

</View>
</View>
</Tooltip>
)}
</View>
))
) : (
<View style={avatarContainerStyles}>
<View style={singleAvatarStyles}>
<UserDetailsTooltip accountID={props.icons[0].id}>
{/* View is necessary for tooltip to show for multiple avatars in LHN */}
<View>
<Avatar
source={props.icons[0].source || props.fallbackIcon}
fill={themeColors.iconSuccessFill}
size={props.isFocusMode ? CONST.AVATAR_SIZE.MID_SUBSCRIPT : CONST.AVATAR_SIZE.SMALLER}
imageStyles={[singleAvatarStyles]}
name={props.icons[0].name}
type={props.icons[0].type}
/>
</View>
</UserDetailsTooltip>
<View style={secondAvatarStyles}>
{props.icons.length === 2 ? (
<UserDetailsTooltip accountID={props.icons[1].id}>
<View>
<Avatar
source={props.icons[1].source || props.fallbackIcon}
fill={themeColors.iconSuccessFill}
size={props.isFocusMode ? CONST.AVATAR_SIZE.MID_SUBSCRIPT : CONST.AVATAR_SIZE.SMALLER}
imageStyles={[singleAvatarStyles]}
name={props.icons[1].name}
type={props.icons[1].type}
/>
</View>
</UserDetailsTooltip>
) : (
<Tooltip text={tooltipTexts.slice(1).join(', ')}>
<View style={[singleAvatarStyles, styles.alignItemsCenter, styles.justifyContentCenter]}>
<Text
selectable={false}
style={props.size === CONST.AVATAR_SIZE.SMALL ? styles.avatarInnerTextSmall : styles.avatarInnerText}
>
{`+${props.icons.length - 1}`}
</Text>
</View>
</Tooltip>
)}
</View>
</View>
</View>
)}
</View>
</>
);
}

Expand Down
10 changes: 8 additions & 2 deletions src/pages/home/report/ReportActionItemCreated.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
import ONYXKEYS from '../../../ONYXKEYS';
import RoomHeaderAvatars from '../../../components/RoomHeaderAvatars';
import ReportWelcomeText from '../../../components/ReportWelcomeText';
import participantPropTypes from '../../../components/participantPropTypes';
import * as ReportUtils from '../../../libs/ReportUtils';
Expand All @@ -18,6 +17,7 @@ import withWindowDimensions, {windowDimensionsPropTypes} from '../../../componen
import compose from '../../../libs/compose';
import withLocalize from '../../../components/withLocalize';
import PressableWithoutFeedback from '../../../components/Pressable/PressableWithoutFeedback';
import MultipleAvatars from '../../../components/MultipleAvatars';
import CONST from '../../../CONST';

const propTypes = {
Expand Down Expand Up @@ -77,7 +77,13 @@ function ReportActionItemCreated(props) {
accessibilityLabel={props.translate('common.details')}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
>
<RoomHeaderAvatars icons={icons} />
<MultipleAvatars
icons={icons}
size={props.isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM}
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
shouldStackHorizontally
shouldDisplayAvatarsInRows={props.isSmallScreenWidth}
maxAvatarsInRow={props.isSmallScreenWidth ? CONST.AVATAR_ROW_SIZE.DEFAULT : CONST.AVATAR_ROW_SIZE.LARGE_SCREEN}
/>
</PressableWithoutFeedback>
<View style={[styles.ph5]}>
<ReportWelcomeText report={props.report} />
Expand Down
1 change: 1 addition & 0 deletions src/pages/workspace/WorkspaceInviteMessagePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ class WorkspaceInviteMessagePage extends React.Component {
size={CONST.AVATAR_SIZE.LARGE}
icons={OptionsListUtils.getAvatarsForAccountIDs(_.values(this.props.invitedEmailsToAccountIDsDraft), this.props.personalDetails)}
shouldStackHorizontally
shouldDisplayAvatarsInRows
secondAvatarStyle={[styles.secondAvatarInline]}
/>
</View>
Expand Down
Loading