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.

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 rendering of timezone info inside chats when the selected language is Spanish #4309

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

aman-atg
Copy link
Contributor

@aman-atg aman-atg commented Jul 29, 2021

Details

(Comment)

Fixed Issues

$ #4218

Tests || ### QA Steps

  1. Login to the app
  2. Change your language to Spanish
  3. Open any chat
  4. Timezone information in the chat will be shown

--->

Tested On

  • Web
  • Mobile Web
  • Android
  • Can't test on these two
  • Desktop
  • iOS

Screenshots

Web

testing.mp4

Mobile Web

mWebTesting.mp4

Desktop

iOS

Android

AndroidTest.mp4

remove unnecessary logic, use preferredLocale
@aman-atg aman-atg requested a review from a team as a code owner July 29, 2021 17:44
@MelvinBot MelvinBot requested review from timszot and removed request for a team July 29, 2021 17:44
componentWillUnmount() {
clearInterval(this.timer);
clearInterval(this.readyTimer);
}

getParticipantLocalTime() {
const reportRecipientTimezone = lodashGet(this.props.participant, 'timezone', {});
moment.locale(this.props.preferredLocale);
Copy link
Member

Choose a reason for hiding this comment

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

This will cause issues with moment() usages. it updates the locale to another user's timezone globally. There are many places where we are dependent on the moment() to get the time.

Do you think it's safe to use it this way?

Copy link
Contributor

Choose a reason for hiding this comment

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

@parasharrajat I haven't reviewed this code yet, in the middle of testing. Can you give me an example of a type of regression this could cause? So far in my testing things are working great and haven't noticed issues with displayed times yet.

Copy link
Member

@parasharrajat parasharrajat Jul 29, 2021

Choose a reason for hiding this comment

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

Please look at ReimbursementAccountPage L174 & CompanyStep L107. Dates could be +1 or -1 based on timezone etc. If your testing shows fine results then all good.

Copy link
Contributor

@timszot timszot left a comment

Choose a reason for hiding this comment

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

Tested this on desktop and iOS, saw no regressions in displayed times.

@timszot timszot merged commit 99063f7 into Expensify:main Jul 29, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.81-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to production in version: 1.0.82-7🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants