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

Fix Group Chat Avatar Offline Feedback in Chat #42088

Merged
merged 10 commits into from
May 28, 2024
Merged

Fix Group Chat Avatar Offline Feedback in Chat #42088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ade046e
add offline feedback for group avatar in report
nexarvo May 13, 2024
bc439be
add offline feedback for group avatar in header
nexarvo May 13, 2024
1bdda1c
add offline feedback for group avatar in LHN
nexarvo May 13, 2024
99e0ed3
remove offline feedback from edit icon
nexarvo May 13, 2024
2432680
remove offline feedback from avatar edit icon in profile setting page
nexarvo May 13, 2024
8848d08
revert offline feedback for avatar in LHN
nexarvo May 13, 2024
ce676eb
fix lint
nexarvo May 13, 2024
c218f42
fix error message style
nexarvo May 14, 2024
d67f0e4
fix lint
nexarvo May 14, 2024
058815a
remove extra view
nexarvo May 23, 2024
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: 2 additions & 3 deletions src/components/AvatarWithImagePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,6 @@ function AvatarWithImagePicker({
<View style={StyleSheet.flatten([styles.alignItemsCenter, style])}>
<View style={styles.w100}>
<OfflineWithFeedback
pendingAction={pendingAction}
errors={errors}
errorRowStyles={errorRowStyles}
style={type === CONST.ICON_TYPE_AVATAR && styles.alignItemsCenter}
Expand All @@ -323,7 +322,7 @@ function AvatarWithImagePicker({
style={[styles.pRelative, avatarStyle]}
ref={anchorRef}
>
<View>
<OfflineWithFeedback pendingAction={pendingAction}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have two OfflineWithFeedback?

Copy link
Contributor Author

@nexarvo nexarvo May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We only want to apply pendingAction to avatar. But if we apply error in the second OfflineWithFeedback then it messes with the overall styles of error message. That's why I removed pendingAction from the first OfflineWithFeedback and it will only responsible to show errors if any. If we only apply OfflineWithFeedback to avatar only then we need to change the style of error message and I tried different padding and margins also but it was not working. The error message overlaps with the title and description as well here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having two OfflineWithFeedback for the same underlaying component feels like a workaround. Can you get ride of one of them and try to fix the error message? (try use styles.w100 if it helps)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @s77rt, I have tried a couple of ways to resolve this issue and some of them worked as well but they broke some other stuff. We will need to refactor the whole AvatarWithImagePicker component to make this work. We are already using multiple OfflineFeedback for error and pending action like in this file:

App/src/components/ReportActionItem/TaskView.tsx

Lines 65 to 71 in d67f0e4

<OfflineWithFeedback
shouldShowErrorMessages
errors={report.errorFields?.editTask ?? report.errorFields?.createTask}
onClose={() => Task.clearTaskErrors(report.reportID)}
errorRowStyles={styles.ph5}
>
<Hoverable>

And for only pendingAction:

App/src/components/ReportActionItem/TaskView.tsx

Lines 94 to 95 in d67f0e4

<OfflineWithFeedback pendingAction={report.pendingFields?.reportName}>
<Text style={styles.taskTitleDescription}>{translate('task.title')}</Text>

My suggestion is we use 2 OfflineFeedback as we are already using this way in other places in the App. Please let me know if there is any better approach for this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nexarvo Let's not block on that. Can you just remove the extra wrapping View that's inside <OfflineWithFeedback />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed extra view in this commit

{source ? (
<Avatar
containerStyles={avatarStyle}
Expand All @@ -336,7 +335,7 @@ function AvatarWithImagePicker({
) : (
<DefaultAvatar />
)}
</View>
</OfflineWithFeedback>
{!disabled && (
<View style={StyleSheet.flatten([styles.smallEditIcon, styles.smallAvatarEditIcon, editIconStyle])}>
<Icon
Expand Down
11 changes: 7 additions & 4 deletions src/pages/home/HeaderView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type {ThreeDotsMenuItem} from '@components/HeaderWithBackButton/types';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import MultipleAvatars from '@components/MultipleAvatars';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
import ParentNavigationSubtitle from '@components/ParentNavigationSubtitle';
import PressableWithoutFeedback from '@components/Pressable/PressableWithoutFeedback';
import ReportHeaderSkeletonView from '@components/ReportHeaderSkeletonView';
Expand Down Expand Up @@ -266,10 +267,12 @@ function HeaderView({
size={defaultSubscriptSize}
/>
) : (
<MultipleAvatars
icons={icons}
shouldShowTooltip={!isChatRoom || isChatThread}
/>
<OfflineWithFeedback pendingAction={report.pendingFields?.avatar}>
<MultipleAvatars
icons={icons}
shouldShowTooltip={!isChatRoom || isChatThread}
/>
</OfflineWithFeedback>
)}
<View style={[styles.flex1, styles.flexColumn]}>
<DisplayNames
Expand Down
32 changes: 17 additions & 15 deletions src/pages/home/report/ReportActionItemCreated.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,21 +68,23 @@ function ReportActionItemCreated(props: ReportActionItemCreatedProps) {
accessibilityLabel={translate('accessibilityHints.chatWelcomeMessage')}
style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth)]}
>
<PressableWithoutFeedback
onPress={() => ReportUtils.navigateToDetailsPage(props.report)}
style={[styles.mh5, styles.mb3, styles.alignSelfStart]}
accessibilityLabel={translate('common.details')}
role={CONST.ROLE.BUTTON}
disabled={shouldDisableDetailPage}
>
<MultipleAvatars
icons={icons}
size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM}
shouldStackHorizontally
shouldDisplayAvatarsInRows={isSmallScreenWidth}
maxAvatarsInRow={isSmallScreenWidth ? CONST.AVATAR_ROW_SIZE.DEFAULT : CONST.AVATAR_ROW_SIZE.LARGE_SCREEN}
/>
</PressableWithoutFeedback>
<OfflineWithFeedback pendingAction={props.report?.pendingFields?.avatar}>
<PressableWithoutFeedback
onPress={() => ReportUtils.navigateToDetailsPage(props.report)}
style={[styles.mh5, styles.mb3, styles.alignSelfStart]}
accessibilityLabel={translate('common.details')}
role={CONST.ROLE.BUTTON}
disabled={shouldDisableDetailPage}
>
<MultipleAvatars
icons={icons}
size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM}
shouldStackHorizontally
shouldDisplayAvatarsInRows={isSmallScreenWidth}
maxAvatarsInRow={isSmallScreenWidth ? CONST.AVATAR_ROW_SIZE.DEFAULT : CONST.AVATAR_ROW_SIZE.LARGE_SCREEN}
/>
</PressableWithoutFeedback>
</OfflineWithFeedback>
<View style={[styles.ph5]}>
<ReportWelcomeText
report={props.report}
Expand Down
8 changes: 2 additions & 6 deletions src/pages/settings/InitialSettingsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import CurrentUserPersonalDetailsSkeletonView from '@components/CurrentUserPerso
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import MenuItem from '@components/MenuItem';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
import {PressableWithFeedback} from '@components/Pressable';
import ScreenWrapper from '@components/ScreenWrapper';
import {ScrollOffsetContext} from '@components/ScrollOffsetContextProvider';
Expand Down Expand Up @@ -407,10 +406,7 @@ function InitialSettingsPage({session, userWallet, bankAccountList, fundList, wa
</PressableWithFeedback>
</Tooltip>
</View>
<OfflineWithFeedback
pendingAction={currentUserPersonalDetails?.pendingFields?.avatar ?? undefined}
style={[styles.mb3, styles.w100]}
>
<View style={[styles.mb3, styles.w100]}>
<AvatarWithImagePicker
isUsingDefaultAvatar={UserUtils.isDefaultAvatar(currentUserDetails?.avatar ?? '')}
source={UserUtils.getAvatar(avatarURL, accountID)}
Expand All @@ -429,7 +425,7 @@ function InitialSettingsPage({session, userWallet, bankAccountList, fundList, wa
fallbackIcon={currentUserDetails?.fallbackIcon}
editIconStyle={styles.smallEditIconAccount}
/>
</OfflineWithFeedback>
</View>
<Text
style={[styles.textHeadline, styles.pre, styles.textAlignCenter]}
numberOfLines={1}
Expand Down
Loading