Implement library to format time/dates for international users. #2075
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hey, @iwiznia, as per your suggestions, I have not imported any locale files and just laid out the groundwork. So, in order for various locales to work, we need to import that manually for now. Let me know if you have any other suggestions. Note: I used locale |
|
recheck |
1 similar comment
|
recheck |
src/libs/DateUtils.js
Outdated
| } | ||
|
|
||
| if (includeTimeZone) { | ||
| retVal = 'timeZone'; |
There was a problem hiding this comment.
Is this going to be equivalent to what we had? Before we appended the timezone to the format ie: Yesterday at 10:20 PST but now it seems we will return a full date when the includeTimeZone param is true, no?
There was a problem hiding this comment.
I think that what we want is:
- Move this override outside this function
- Remove the
timeZonehandling from it - After computing the calendar date, if
includeTimeZonewas passed, append the timezone to the generated string.
Right?
There was a problem hiding this comment.
Aaahh, you are correct! Thanks, will make the changes and push.
src/libs/DateUtils.js
Outdated
| } | ||
| function timestampToDateTime(locale, timestamp, includeTimeZone = false) { | ||
| const date = getLocalMomentFromTimestamp(locale, timestamp); | ||
| const TZ = '[UTC]Z'; |
There was a problem hiding this comment.
IMO it is clearer if you do this
| const TZ = '[UTC]Z'; | |
| const TZ = includeTimeZone ? ' [UTC]Z' : ''; |
Then remove the if (includeTimeZone) { below and in each of the calendar key options do for example [Today at] LT${TZ} and remove the call to calendar that does not include ${TZ}.
Also, I think that our guidelines say this should be named tz?
There was a problem hiding this comment.
Goshhh! Why didn't I think of that, I made the requested change. Let me know if there is anything else.
| * @param {Number} timestamp | ||
| * | ||
| * @returns {Moment} | ||
| * | ||
| * @private | ||
| */ | ||
| function getLocalMomentFromTimestamp(timestamp) { | ||
| function getLocalMomentFromTimestamp(locale, timestamp) { | ||
| moment.locale(locale); |
There was a problem hiding this comment.
If I understand correctly, the modal.locale method takes a language (like English = "en"). Calling the variable that we pass to it locale is confusing.
I think we should rename the variable name locale to lang just for clarity. That way, when you're calling something like timestampToDateTime you know the first argument that you have to pass is a language and not a location.
There was a problem hiding this comment.
No, locale is the better name, since this is not a language, but a locale, which is something like en-US.
There was a problem hiding this comment.
Really? Because we are only call it using a language - 'en'.
{DateUtils.timestampToDateTime('en', props.timestamp)}
Either way, it's not a biggie. I just think it will end up being a bit confusing.
There was a problem hiding this comment.
Yeah, that's just an example so this PR could be tested. We will be replacing that call with the preferred locale later.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Details
timestampToDateTimeandtimestampToRelative.timestampToRelativefunction implementation withmoment().fromNow()method.timestampToDateTimefunction with the momentcalendar()method with the exact configuration as that of the current implementation in Expensify.Fixed Issues
#1856
Tests
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android