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.

Sign up for GitHub

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

Enable avatar preview for workspace & thread replies #24425

Merged
merged 7 commits into from
Aug 15, 2023
Merged

Enable avatar preview for workspace & thread replies #24425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c45580
Enable avatar preview for workspace & thread replies
spcheema Aug 11, 2023
9d4d792
Update props docs
spcheema Aug 11, 2023
ffcf575
Avoid duplicate func call
spcheema Aug 15, 2023
0a98249
Refactor & cleanup
spcheema Aug 15, 2023
0282519
Update src/components/RoomHeaderAvatars.js
spcheema Aug 15, 2023
301ad35
Replace static string with const
spcheema Aug 15, 2023
8ff1d2f
Fix avatar hover issue in the case of multiple avatars
spcheema Aug 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/components/AttachmentModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ const propTypes = {
...withLocalizePropTypes,

...windowDimensionsPropTypes,

/** Denotes whether it is a workspace avatar or not */
isWorkspaceAvatar: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -86,6 +89,7 @@ const defaultProps = {
onModalShow: () => {},
onModalHide: () => {},
onCarouselAttachmentChange: () => {},
isWorkspaceAvatar: false,
};

function AttachmentModal(props) {
Expand Down Expand Up @@ -345,6 +349,7 @@ function AttachmentModal(props) {
isAuthTokenRequired={isAuthTokenRequired}
file={file}
onToggleKeyboard={updateConfirmButtonVisibility}
isWorkspaceAvatar={props.isWorkspaceAvatar}
/>
)
)}
Expand Down
17 changes: 16 additions & 1 deletion src/components/Attachments/AttachmentView/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import variables from '../../../styles/variables';
import AttachmentViewImage from './AttachmentViewImage';
import AttachmentViewPdf from './AttachmentViewPdf';
import addEncryptedAuthTokenToURL from '../../../libs/addEncryptedAuthTokenToURL';

import * as StyleUtils from '../../../styles/StyleUtils';
import {attachmentViewPropTypes, attachmentViewDefaultProps} from './propTypes';

const propTypes = {
Expand All @@ -34,6 +34,9 @@ const propTypes = {
/** Extra styles to pass to View wrapper */
// eslint-disable-next-line react/forbid-prop-types
containerStyles: PropTypes.arrayOf(PropTypes.object),

/** Denotes whether it is a workspace avatar or not */
isWorkspaceAvatar: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -42,6 +45,7 @@ const defaultProps = {
shouldShowLoadingSpinnerIcon: false,
onToggleKeyboard: () => {},
containerStyles: [],
isWorkspaceAvatar: false,
};

function AttachmentView({
Expand All @@ -57,16 +61,27 @@ function AttachmentView({
onToggleKeyboard,
translate,
isFocused,
isWorkspaceAvatar,
}) {
const [loadComplete, setLoadComplete] = useState(false);

// Handles case where source is a component (ex: SVG)
if (_.isFunction(source)) {
let iconFillColor = '';
let additionalStyles = [];
if (isWorkspaceAvatar) {
const defaultWorkspaceAvatarColor = StyleUtils.getDefaultWorkspaceAvatarColor(file.name);
iconFillColor = defaultWorkspaceAvatarColor.fill;
additionalStyles = [defaultWorkspaceAvatarColor];
}

return (
<Icon
src={source}
height={variables.defaultAvatarPreviewSize}
width={variables.defaultAvatarPreviewSize}
fill={iconFillColor}
additionalStyles={additionalStyles}
/>
);
}
Expand Down
72 changes: 55 additions & 17 deletions src/components/RoomHeaderAvatars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import Avatar from './Avatar';
import themeColors from '../styles/themes/default';
import * as StyleUtils from '../styles/StyleUtils';
import avatarPropTypes from './avatarPropTypes';
import PressableWithoutFocus from './Pressable/PressableWithoutFocus';
import * as UserUtils from '../libs/UserUtils';
import AttachmentModal from './AttachmentModal';

const propTypes = {
icons: PropTypes.arrayOf(avatarPropTypes),
Expand All @@ -25,14 +28,31 @@ function RoomHeaderAvatars(props) {

if (props.icons.length === 1) {
return (
<Avatar
source={props.icons[0].source}
imageStyles={[styles.avatarLarge]}
fill={themeColors.iconSuccessFill}
size={CONST.AVATAR_SIZE.LARGE}
name={props.icons[0].name}
type={props.icons[0].type}
/>
<AttachmentModal
headerTitle={props.icons[0].name}
source={UserUtils.getFullSizeAvatar(props.icons[0].source, props.icons[0].id)}
isAuthTokenRequired
Copy link
Contributor

Choose a reason for hiding this comment

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

@spcheema @aimane-chnaif

I have a query regarding the isAuthTokenRequired flag set to true for the AttachmentModal component. This flag's setting becomes particularly interesting when compared with the Avatar component's behavior. Both AttachmentModal and Avatar utilize the same image source (props.icons[0].source), yet Avatar manages to load its image without requiring authentication.

This leads me to wonder if there are specific scenarios where authentication is indeed necessary for loading an avatar image. Understanding this is crucial because if authentication is not required in these cases, we might be exposing our authentication tokens to external addresses unnecessarily.

Furthermore, it’s important to consider future updates, particularly regarding the Image component's adaptation to use header-based authentication. Such changes can introduce complexities due to browser security features like CORS (Cross-Origin Resource Sharing). Since avatar sources are not hosted on the same origin, this could potentially lead to CORS errors, disrupting the user experience.

Could you provide insights on the necessity of the isAuthTokenRequired flag in this context? Understanding the rationale behind this decision will help us ensure both secure and efficient handling of image sources across different components.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also interested - AFAIK the auth tokens were only needed for chat attachments, since these obviously need to be private (only accessible by the participants of a report where the chat attachment was sent). I don't know of / remember a reason we would need any avatars to be private & require auth tokens to fetch

isWorkspaceAvatar={props.icons[0].type === CONST.ICON_TYPE_WORKSPACE}
originalFileName={props.icons[0].name}
>
{({show}) => (
<PressableWithoutFocus
style={[styles.noOutline]}
onPress={show}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityLabel={props.icons[0].name}
>
<Avatar
source={props.icons[0].source}
imageStyles={[styles.avatarLarge]}
fill={themeColors.iconSuccessFill}
size={CONST.AVATAR_SIZE.LARGE}
name={props.icons[0].name}
type={props.icons[0].type}
/>
</PressableWithoutFocus>
)}
</AttachmentModal>
);
}

Expand All @@ -45,27 +65,45 @@ function RoomHeaderAvatars(props) {
StyleUtils.getAvatarStyle(CONST.AVATAR_SIZE.LARGE_BORDERED),
];
return (
<View pointerEvents="none">
<View pointerEvents="box-none">
<View style={[styles.flexRow, styles.wAuto, styles.ml3]}>
{_.map(iconsToDisplay, (icon, index) => (
<View
key={`${icon.source}${index}`}
style={[styles.justifyContentCenter, styles.alignItemsCenter]}
>
<Avatar
source={icon.source}
fill={themeColors.iconSuccessFill}
size={CONST.AVATAR_SIZE.LARGE}
containerStyles={[...iconStyle, StyleUtils.getAvatarBorderRadius(CONST.AVATAR_SIZE.LARGE_BORDERED, icon.type)]}
name={icon.name}
type={icon.type}
/>
<AttachmentModal
headerTitle={icon.name}
source={UserUtils.getFullSizeAvatar(icon.source, icon.id)}
isAuthTokenRequired
originalFileName={icon.name}
isWorkspaceAvatar={icon.type === CONST.ICON_TYPE_WORKSPACE}
>
{({show}) => (
<PressableWithoutFocus
style={[styles.mln4, StyleUtils.getAvatarBorderRadius(CONST.AVATAR_SIZE.LARGE_BORDERED, icon.type)]}
onPress={show}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityLabel={icon.name}
>
<Avatar
source={icon.source}
fill={themeColors.iconSuccessFill}
size={CONST.AVATAR_SIZE.LARGE}
containerStyles={[...iconStyle, StyleUtils.getAvatarBorderRadius(CONST.AVATAR_SIZE.LARGE_BORDERED, icon.type)]}
name={icon.name}
type={icon.type}
/>
</PressableWithoutFocus>
)}
</AttachmentModal>
{index === CONST.REPORT.MAX_PREVIEW_AVATARS - 1 && props.icons.length - CONST.REPORT.MAX_PREVIEW_AVATARS !== 0 && (
<>
<View
style={[
styles.roomHeaderAvatarSize,
styles.roomHeaderAvatar,
styles.mln4,
...iconStyle,
StyleUtils.getAvatarBorderRadius(CONST.AVATAR_SIZE.LARGE_BORDERED, icon.type),
styles.roomHeaderAvatarOverlay,
Expand Down
1 change: 0 additions & 1 deletion src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2358,7 +2358,6 @@ const styles = {

roomHeaderAvatar: {
backgroundColor: themeColors.appBG,
marginLeft: -16,
borderRadius: 100,
borderColor: themeColors.componentBG,
borderWidth: 4,
Expand Down
4 changes: 4 additions & 0 deletions src/styles/utilities/spacing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ export default {
marginLeft: 16,
},

mln4: {
marginLeft: -16,
},

ml5: {
marginLeft: 20,
},
Expand Down