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
Fix for: Room name rename text is translated in chat but not translated to Spanish in LHN #20479
Conversation
@neil-marcellini @abdulrahuman5196 One of you needs to 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] |
I have for now just included my original proposal with some minor alterations. Personally I think it looks good with the same text that we have in the chat, is more congruent that way and there seems to be enough space for it, but let me know what you think. Something that I did notice is that when you change the text to Spanish the Happy to fix this too even if it it may be a little out of scope. |
@abdulrahuman5196 Ok, so you're suggesting we create a new translation key for |
Yes. Thats my suggestion. But During the slack request for translation confirmation itself you can also check if any other text is good. I think this is best IMO but we can always double check with others on the string if you think other is better. No issues on that. |
@abdulrahuman5196 Ok, I've asked in Slack. |
@abdulrahuman5196 This commit should fix "You" not being translated in the chat as I mentioned here. |
@abdulrahuman5196 Updated as per your suggestions. |
It seems in fresh login we are not getting lastMessageText and 'No activity Yet' or 'Beginning of ' text is shown in case of different actions like renamed, IOU, tasks. I think we would need a more broader fix to support this similar places/action in the LHN sidebar subtitle to what we had discussed in the slack here - https://expensify.slack.com/archives/C01GTK53T8Q/p1686326271370549?thread_ts=1686301855.618179&cid=C01GTK53T8Q (Could have some backend changes as well) |
Reviewer Checklist
Screenshots/VideosWebUntitled.27.mp4Mobile Web - ChromeUntitled.24.mp4Mobile Web - SafariUntitled.28.mp4DesktopUntitled.26.mp4iOSUntitled.25.mp4AndroidUntitled.29.mp4 |
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.
Changes looks good and works well. And reviewers check is also complete.
All yours @neil-marcellini
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.
Looks great, but I want to understand why we made changes to the RenameAction.
const currentUserAccountID = lodashGet(props.currentUserPersonalDetails, 'accountID', ''); | ||
const userDisplayName = lodashGet(props.action, ['person', 0, 'text']); | ||
const actorAccountID = lodashGet(props.action, 'actorAccountID', ''); | ||
const displayName = actorAccountID === currentUserAccountID ? `${props.translate('common.you')}` : `${userDisplayName}`; |
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.
Why did we need to make this change to set "You" instead of the user's display name? It seems unnecessary.
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.
@neil-marcellini
This was not part of the issue which we are fixing in LHN, this was a related translation issue where 'you' text was not translated in the chat page for the rename action. @Ollyws mentioned this issue and lended to fix it as well.
Since it was a similar translation issue during room rename action, I was ok with the extra fix as well. Let me know if you think otherwise.
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.
@neil-marcellini I originally mentioned here, I noticed that the 'You' in the chat was not localised to Spanish as you can can see in the linked comment, so I opted to fix that too.
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.
Ah ok I missed that. Thanks for explaining. Please add tests for that part as well.
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.
Please fix merge conflicts and then it should be good to go. Please message me on NewDot when it's updated for a quicker review.
const currentUserAccountID = lodashGet(props.currentUserPersonalDetails, 'accountID', ''); | ||
const userDisplayName = lodashGet(props.action, ['person', 0, 'text']); | ||
const actorAccountID = lodashGet(props.action, 'actorAccountID', ''); | ||
const displayName = actorAccountID === currentUserAccountID ? `${props.translate('common.you')}` : `${userDisplayName}`; |
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.
Ah ok I missed that. Thanks for explaining. Please add tests for that part as well.
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.
Great thanks! Abdul's videos show the new test step too so I think we're good to go.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.28-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
Details
Fixed Issues
$ #20124
PROPOSAL: #20124 (comment)
Tests
Offline tests
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 methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
MacOS_Chrome.mp4
Mobile Web - Chrome
Android_Chrome.mp4
Mobile Web - Safari
iOS_Safari.mp4
Desktop
MacOS_Desktop.mp4
iOS
iOS_Native.mp4
Android
Android_Native.mp4