Page MenuHomePhabricator

[Cashtab] Change date format
ClosedPublic

Authored by kieran709 on Dec 8 2021, 22:33.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABCf8759e43b370: [Cashtab] Change date format
Summary

Changed date strings to match request in task T2058. Each instance of toLocaleDate() has been updated to include an options parameter which presents the date in the MMM-dd-yyyy format.

Test Plan

cd web / cashtab && npm start
In the wallet tab
Ensure Tx dates are formatted correctly.
Navigate to the eTokens tab
Select an eToken
Ensure genesis date is formatted correctly.
In browser settings, change locale and run through test plan again.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
update-date-strings
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17659
Build 35144: Build Diffcashtab-tests
Build 35143: arc lint + arc unit

Event Timeline

Can you wrap this inside a formatDate() function in validation.js (see formatFiatBalance() which formats the currency string based on locale input params) which will then allow you to add unit tests in validation.test.js to validate its behavior when passing in null, undefined, blank strings, invalid date formats..etc? This will allow other components to confidently re-use this battle tested function later on.

bytesofman added inline comments.
web/cashtab/src/components/Send/SendToken.js
386 ↗(On Diff #31332)

Keep options here as an object instead of a variable, confusing to define it so far from where it is used, and no need to use the variable since it is only used once.

web/cashtab/src/components/Wallet/Tx.js
176 ↗(On Diff #31332)

Call this dateFormatOptions to improve code readability

This revision now requires changes to proceed.Dec 14 2021, 19:00

Created function which checks object for either timestampUnix or blocktime, and formats the date based on locale + options. I'd like to simplify it a bit but I am not exactly sure how to proceed. Once the function is finalized I will add unit testing.

Also, planning to change the param 'tokenObject' name because it is not descriptive.

Instead of trying to handle all of the date related code, simplified the function to just accept a date string per feedback from bytesofman.

Looks serviceable. Please add unit tests in validation.test.js

This revision now requires changes to proceed.Dec 17 2021, 00:31

Added unit tests per review feedback.

bytesofman added inline comments.
web/cashtab/src/utils/validation.js
180 ↗(On Diff #31450)

Because you are using return here, you don't need to use an if...else block

if (dateString) {
            return new Date(dateString * 1000).toLocaleDateString(
                userLocale,
                options,
            );
        } else {
            return new Date().toLocaleDateString(userLocale, options);
        }

is the same as

if (dateString) {
            return new Date(dateString * 1000).toLocaleDateString(
                userLocale,
                options,
            );
        } 
            return new Date().toLocaleDateString(userLocale, options);

Use the latter

This revision now requires changes to proceed.Dec 29 2021, 18:39

rebasing and responding to review feedback

bytesofman requested changes to this revision.Jan 3 2022, 13:46
bytesofman added inline comments.
web/cashtab/src/utils/validation.js
217 ↗(On Diff #31593)

Move the definition of userLocale into the try block. Could be edge case where the navigator object is unavailable.

This revision now requires changes to proceed.Jan 3 2022, 13:46

Responding to review feedback

bytesofman requested changes to this revision.Jan 3 2022, 15:59

Please update the test plan to include testing with different locales.

I'm running into an issue like this:

  1. Load page in Google Chrome with en-US locale and then right click --> translate page, say to Indonesian
  2. Note that dates on Wallet homescreen of tx history update
  3. Click Token screen
  4. Click any other screen
  5. App crashes

Please debug and update test plan to include testing in multiple locales

web/cashtab/src/utils/__tests__/validation.test.js
232 ↗(On Diff #31605)

Expected test answers should be resolved, e.g. here it should be "Dec 16, 2021"

This revision now requires changes to proceed.Jan 3 2022, 15:59
kieran709 edited the test plan for this revision. (Show Details)

Changed test plan and validation test per review feedback.

This revision is now accepted and ready to land.Jan 5 2022, 17:00
This revision was automatically updated to reflect the committed changes.