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

renderSpan: add more time intervals #5568

Merged

Conversation

j-s-n
Copy link
Contributor

@j-s-n j-s-n commented Oct 21, 2024

Summary

Added years, months, and days to renderSpan
Estimated years to 365 days and months to 30 days.

Testing Plan

Lock and unlock a wallet with flags for various time intervals.
Confirmed it returns the highest 2 time units (year & month, month & day, etc.)

Documentation

N/A

Breaking Change

N/A

@j-s-n j-s-n requested a review from a team as a code owner October 21, 2024 19:13
@j-s-n j-s-n force-pushed the renderSpan-add-more-time-intervals branch from 109fdd6 to 4c4e0dc Compare November 25, 2024 23:57
@NullSoldier
Copy link
Contributor

I think we should add a few tests in the test file for these new cases.

@j-s-n
Copy link
Contributor Author

j-s-n commented Nov 26, 2024

I think we should add a few tests in the test file for these new cases.

added tests for renderSpan and renderEstimate. Updated renderEstimate to return largest 3 intervals (y, M, d) for longer periods and renderSpan outputs largest 2 intervals (e.g. y, M / M, d / d, h)

@NullSoldier
Copy link
Contributor

Ok the PR looks good. Assuming the tests pass we should merge this in.

@j-s-n j-s-n force-pushed the renderSpan-add-more-time-intervals branch from aa7f611 to 3130101 Compare January 4, 2025 22:49
@NullSoldier NullSoldier merged commit 3a87bda into iron-fish:staging Jan 4, 2025
12 checks passed
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.

2 participants