-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
update beginningOfChatHistory translation #49919
Conversation
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@@ -1,53 +1,52 @@ | |||
import React, {useMemo} from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,53 +1,52 @@ | |||
import React, {useMemo} from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Archived Workspace Invoice Chat
Staging | PR |
---|---|
This is a Staging vs PR before / after of an archived workspace invoice chat, the way you get an archived workspace invoice chat is by:
- Create new WS what is able to send invoices.
- FAB > Send Invoice > Select WS and Send.
- Settings > Workspaces > Delete WS.
- Notice the archived workspace invoice chat welcome text.
Observe the discrepancies between Staging and PR screenshots, missing (archived) and the addition of WS name.
@nkdengineer Once the 🔴 Request changes are addressed and conflicts are resolved, will review again. Thanks! |
Co-authored-by: Kevin Brian Bader <56457735+ikevin127@users.noreply.github.com>
Co-authored-by: Kevin Brian Bader <56457735+ikevin127@users.noreply.github.com>
@ikevin127 Updated these cases above. |
src/libs/SidebarUtils.ts
Outdated
} else { | ||
// Message for user created rooms or other room types. | ||
welcomeMessage.phrase1 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryUserRoomPartOne'); | ||
welcomeMessage.phrase2 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryUserRoomPartTwo'); | ||
welcomeMessage.phrase2 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryUserRoomPartTwo', {displayName: ReportUtils.getPolicyName(report)}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this line using ReportUtils.getPolicyName(report)
for the displayName
does not yield the correct result because for WS #rooms it will say that the room was created by the {workspaceName} when it should be the name of the user that created the room aka the Workspace owner.
The reason I was able to point this out was because the copy says:
This chat room is for anything #[roomName] related. It was created by [displayName].
And this is how the workspace room looks on my side, it should say Zerax
instead of ZeraWS
:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is in the report data we cannot know who is the user that created the room.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkdengineer Cool, thanks for the context. I'll bring this up with the team in the issue to see whether we leave it as is or change it.
♻️ Will proceed with the checklist in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkdengineer Looks like out of the options provided by me in #47427 (comment), the decision was to simply remove the It was created by [displayName].
part from the #room copy.
♻️ I will move forward with completing the checklist today as everything else looks good, and whenever you get a chance please remove that part. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ikevin127 I updated.
@nkdengineer Thanks for fixing the previous issues, this looks good overall 👍 I found 2 other minor issues -> once the latest 🔴 Request changes are addressed will review again and move forward with the PR Reviewer Checklist if everything looks good. Thank you! |
Co-authored-by: Kevin Brian Bader <56457735+ikevin127@users.noreply.github.com>
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: Nativeios.mp4iOS: mWeb Safariios-mweb.mp4MacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice optimization opportunity (such that ReportWelcomeText.tsx
will only re-render if personalDetails for one of the report participants change, not any personalDetails for any user)
diff --git a/src/components/ReportWelcomeText.tsx b/src/components/ReportWelcomeText.tsx
index 12708a7acbf..facb20f8a6f 100644
--- a/src/components/ReportWelcomeText.tsx
+++ b/src/components/ReportWelcomeText.tsx
@@ -30,7 +30,6 @@ type ReportWelcomeTextProps = {
function ReportWelcomeText({report, policy}: ReportWelcomeTextProps) {
const {translate} = useLocalize();
const styles = useThemeStyles();
- const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID || -1}`);
@@ -42,7 +41,10 @@ function ReportWelcomeText({report, policy}: ReportWelcomeTextProps) {
const isDefault = !(isChatRoom || isPolicyExpenseChat || isSelfDM || isInvoiceRoom || isSystemChat);
const participantAccountIDs = ReportUtils.getParticipantsAccountIDsForDisplay(report, undefined, undefined, true);
const isMultipleParticipant = participantAccountIDs.length > 1;
- const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(OptionsListUtils.getPersonalDetailsForAccountIDs(participantAccountIDs, personalDetails), isMultipleParticipant);
+ const [personalDetails = []] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {
+ selector: (personalDetailsList) => OptionsListUtils.getPersonalDetailsForAccountIDs(participantAccountIDs, personalDetailsList),
+ });
+ const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(personalDetails, isMultipleParticipant);
const welcomeMessage = SidebarUtils.getWelcomeMessage(report, policy);
const moneyRequestOptions = ReportUtils.temporary_getMoneyRequestOptions(report, policy, participantAccountIDs);
const filteredOptions = moneyRequestOptions.filter(
@roryabraham We have a problem if we do that, the selector will be called again only if the |
Oh, interesting. Well we can merge as-is (after all, we didn't have a selector before). But just to sate my curiosity, does it work to wrap the call in const [personalDetails = []] = useMemo(
() => useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {
selector: (personalDetailsList) => OptionsListUtils.getPersonalDetailsForAccountIDs(participantAccountIDs, personalDetailsList)
}),
[participantAccountIDs]
); |
@roryabraham I tested and we can't use |
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
const shouldShowUsePlusButtonText = | ||
(moneyRequestOptions.includes(CONST.IOU.TYPE.PAY) || | ||
moneyRequestOptions.includes(CONST.IOU.TYPE.SUBMIT) || | ||
moneyRequestOptions.includes(CONST.IOU.TYPE.TRACK) || | ||
moneyRequestOptions.includes(CONST.IOU.TYPE.SPLIT)) && | ||
!isPolicyExpenseChat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkdengineer Is there a reason for adding this !isPolicyExpenseChat
condition?
beginningOfChatHistory: 'This chat is with ', | ||
beginningOfChatHistoryPolicyExpenseChatPartOne: 'This is where ', | ||
beginningOfChatHistoryPolicyExpenseChatPartTwo: ' will submit expenses to ', | ||
beginningOfChatHistoryPolicyExpenseChatPartThree: ' workspace. Just use the + button.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s77rt The reason is we already have Just use the + button.
text here
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.0.46-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.46-5 🚀
|
Details
Update the beginningOfChatHistory copy in all rooms to be clearer and more concise, as well as remove any noise around features that don’t lead to conversion.
Fixed Issues
$ #47427
PROPOSAL: #47427 (comment)
Tests
Offline tests
Same as above
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop