-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
get a little more accurate with month length #3981
Conversation
Your Render PR Server URL is https://social-app-pr-3981.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cp0mil21hbls73b00glg. |
|
I think there might be a typo in the second sentence above, I'm guessing it should be: "i.e. 1 month and 29 days should show |
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.
LGTM — we could consider a library for this also
@@ -151,7 +155,32 @@ describe('ago', () => { | |||
new Date().setMinutes(new Date().getMinutes() - 10), | |||
new Date().setHours(new Date().getHours() - 1), | |||
new Date().setDate(new Date().getDate() - 1), | |||
new Date().setDate(new Date().getDate() - 20), |
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.
Don't need to change in this PR, I realize it was already set up like this, but good tip for tests: each of these should be written as an it
clause, with a description, so when one breaks we know exactly which failed. Written like this, one failure fails the whole thing, and determining which failed is harder.
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.
Also it's hard to review bc idk which lines up with which :P
ha yea, that's what i meant 😅 |
Why
Because we are assuming months have 28 days, we're starting to see some cases like this:
Obviously, we know that a year cannot have 13 months in it, but we're starting to encounter cases like this because...well it was just Bluesky's "mainstream" birthday!
So, what do we want to do instead?
29d
1mo
1mo
in most cases.2mo
12mo
but instead begin showing the full date. I.e. 11 months and 29 days should show4/20/24
Test Plan
I have added a variety of test cases to
string.test.ts
. Remember, this still is not precise, but it more closely resembles reality - especially as we get into the later months or as in this case the "13 months" case. If we truly want to be precise,we would either need to write some additional (annoying) logic or use a library like dayjs which is a little overkill for this purpose.
Of course feel free to tweak this however you feel like.