Webclient: Render account age in days and years #4497
Conversation
chrisfinigan
commented
Dec 12, 2021
•
chrisfinigan
commented
- Render account age as days and years
- Fix double quotes for linter
- Add utils/convertTime and tests
|
This actually looks really good, very lightweight and simple, plus i definitely appreciate the included unit tests. However, timestamp formatting is one of those foundational pieces that I think i would prefer left to a library, both for robustness in functionality as well as keeping us from reinventing wheels. Your code here could easily be extended to account for minutes for game age, but it will fall short when we look to represent message timestamps w/ timezone considered. MomentJS was recently deprecated, and based on this article, Luxon seems to be a winning alternative. I took a look at their docs, it looks very promising and lightweight. I was thinking we'd probably create a wrapper service that produces the type of time strings we need, i.e. renderAccountAge, renderGameAge, renderMessageTimestamp, etc.. https://moment.github.io/luxon/#/ |
|
makes sense, ill work on adding Luxon instead in a timeService |
|
Added prettier config for auto formating, husky hook to run "npm run golden" pre-push and use new timeService |
| "postinstall": "run-script-os", | ||
| "postinstall:windows": "powershell .\\copy_shared_files.ps1", | ||
| "postinstall": "run-script-os && npm run prepare", | ||
| "postinstall:windows": "powershell ./copy_shared_files.ps1", |
There was a problem hiding this comment.
Windows is going to want the forward slash, which we need to escape as we had it.
There was a problem hiding this comment.
I was having issues running postinstall with the forward slash. On windows powershell, it would fail with the double forward slash and pass with the change. Change is working with Cygwin too (bash).
webclient/package.json
Outdated
| @@ -40,7 +41,13 @@ | |||
| "eject": "react-scripts eject", | |||
| "lint": "eslint \"./**/*.{ts,tsx}\"", | |||
| "lint:fix": "eslint \"./**/*.{ts,tsx}\" --fix", | |||
| "golden": "npm run lint && npm run test" | |||
| "golden": "npm run lint && npm run test", | |||
| "prepare": "cd .. && husky install webclient/.husky" | |||
There was a problem hiding this comment.
Does this not work? Also, will this command work on windows?
| "prepare": "cd .. && husky install webclient/.husky" | |
| "prepare": "husky install ./.husky" |
There was a problem hiding this comment.
It might make sense to add this command to the env-specific pre-install copy files.
There was a problem hiding this comment.
husky install has to be ran in the same directory as the .git/ hence the cd. I have moved the .husky up to the parent directory. This is working on windows with powershell and cygwin (bash)
| import { DateTime } from 'luxon'; | ||
|
|
||
| class TimeService { | ||
| renderAccountAge(seconds: number) { |
There was a problem hiding this comment.
Does this support < 1 Day? I dont think we care about minutes/hours/seconds for account age, tho i think game age starts with < 1 minute.
There was a problem hiding this comment.
Currently, for less than one days, no text is returned. I am adding a test case to show this
| @@ -30,8 +31,8 @@ | |||
| "typescript": "^4.4.4" | |||
| }, | |||
| "scripts": { | |||
| "postinstall": "run-script-os", | |||
| "postinstall:windows": "powershell .\\copy_shared_files.ps1", | |||
| "postinstall": "run-script-os && npm run prepare", | |||
There was a problem hiding this comment.
I think the code to run whats in prepare should probably just go in the copy_shared_files (maybe change the name to something more relevant to the extended usage). We use run-script-os to run the prepare scripts that are tailored for each specific environment.
Any thoughts @ZeldaZach?
|
this seems to work as expected and shows the age the same as it does in the latest client version. leap years correctly display as they should, similar to the client now. tests should also test for leap years and preferably not for the generated string but for the numbers created ( otherwise it'll become hard to translate later ). if we can also have accounts created on the same day display as 0 days we can merge this pr. |
tooomm
changed the title
seavor
added
the
App - Webclient
