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

Improve report preview action #20941

Merged
merged 7 commits into from
Jun 22, 2023
Merged

Improve report preview action #20941

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fc21c08
Show workspace avatar and name for report preview action when workspa…
cristipaval Jun 16, 2023
9287b19
Merge remote-tracking branch 'origin/main' into cristi_workspace-repo…
cristipaval Jun 21, 2023
38ad14e
don't show the actor name in sidebar when last report action actor is…
cristipaval Jun 21, 2023
f01a341
Fix tooltip text for report actions where the actor is the workspace
cristipaval Jun 21, 2023
fd48471
Run prettier
cristipaval Jun 21, 2023
4e0fdcc
Address feedback after review.
cristipaval Jun 21, 2023
46bf1b9
Fix click handler on avatar and display name for report previews in w…
cristipaval Jun 21, 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
3 changes: 2 additions & 1 deletion src/libs/SidebarUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,8 @@ function getOptionData(reportID) {
}
: null;
}
let lastMessageText = hasMultipleParticipants && lastActorDetails && Number(lastActorDetails.accountID) !== currentUserAccountID ? `${lastActorDetails.displayName}: ` : '';
let lastMessageText =
hasMultipleParticipants && lastActorDetails && lastActorDetails.accountID && Number(lastActorDetails.accountID) !== currentUserAccountID ? `${lastActorDetails.displayName}: ` : '';
lastMessageText += report ? lastMessageTextFromReport : '';

if (result.isArchivedRoom) {
Expand Down
20 changes: 12 additions & 8 deletions src/pages/home/report/ReportActionItemSingle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,13 @@ const showUserDetails = (accountID) => {
};

function ReportActionItemSingle(props) {
const actorEmail = lodashGet(props.action, 'actorEmail', '').replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '');
const actorAccountID = props.action.actorAccountID;
const {avatar, displayName, pendingFields} = props.personalDetailsList[actorAccountID] || {};
const avatarSource = UserUtils.getAvatar(avatar, actorAccountID);
const isWorkspaceActor = ReportUtils.isPolicyExpenseChat(props.report) && !actorAccountID;
const actorDetails = props.personalDetailsList[actorAccountID] || {};
const displayName = isWorkspaceActor ? ReportUtils.getPolicyName(props.report) : actorDetails.displayName;
const actorHint = isWorkspaceActor ? displayName : lodashGet(props.action, 'actorEmail', '').replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question @cristipaval : why are we only using displayName if isWorkspaceActor is true here? It looks like we set a displayName no matter what, so can't we just use that as an actorHint no matter what?

I'm working on removing actorEmail from everywhere since we should have actorAccountID everywhere that we have actorEmail - so I'm hoping we can just use displayName here always

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, displayName would be great. The thing is that this has also to be updated in Web-E 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.

Groovy!

trueeeee our plan is to basically stop using it in App first, then stop sending from web-e 👍

const pendingFields = isWorkspaceActor ? {} : actorDetails.pendingFields;
const avatarSource = isWorkspaceActor ? ReportUtils.getWorkspaceAvatar(props.report) : UserUtils.getAvatar(actorDetails.avatar, actorAccountID);

// Since the display name for a report action message is delivered with the report history as an array of fragments
// we'll need to take the displayName from personal details and have it be in the same format for now. Eventually,
Expand All @@ -90,16 +93,17 @@ function ReportActionItemSingle(props) {
style={[styles.alignSelfStart, styles.mr3]}
onPressIn={ControlSelection.block}
onPressOut={ControlSelection.unblock}
disabled={isWorkspaceActor}
cristipaval marked this conversation as resolved.
Show resolved Hide resolved
onPress={() => showUserDetails(actorAccountID)}
accessibilityLabel={actorEmail}
accessibilityLabel={actorHint}
accessibilityRole="button"
>
<OfflineWithFeedback pendingAction={lodashGet(pendingFields, 'avatar', null)}>
{props.shouldShowSubscriptAvatar ? (
<SubscriptAvatar
mainAvatar={{source: avatarSource, type: CONST.ICON_TYPE_AVATAR}}
secondaryAvatar={ReportUtils.getIcons(props.report, {})[props.report.isOwnPolicyExpenseChat ? 0 : 1]}
mainTooltip={actorEmail}
mainAvatar={{source: avatarSource, type: isWorkspaceActor ? CONST.ICON_TYPE_WORKSPACE : CONST.ICON_TYPE_AVATAR, name: displayName}}
secondaryAvatar={isWorkspaceActor ? {} : ReportUtils.getIcons(props.report, {})[props.report.isOwnPolicyExpenseChat ? 0 : 1]}
mainTooltip={actorHint}
secondaryTooltip={ReportUtils.getPolicyName(props.report)}
noMargin
/>
Expand All @@ -123,7 +127,7 @@ function ReportActionItemSingle(props) {
onPressIn={ControlSelection.block}
onPressOut={ControlSelection.unblock}
onPress={() => showUserDetails(actorAccountID)}
accessibilityLabel={actorEmail}
accessibilityLabel={actorHint}
accessibilityRole="button"
>
{_.map(personArray, (fragment, index) => (
Expand Down