-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
Duration frontend implementation #1350
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1350 +/- ##
=======================================
Coverage 93.58% 93.58%
=======================================
Files 115 115
Lines 4598 4598
=======================================
Hits 4303 4303
Misses 295 295
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
This is very cool!
-
I played with the functionality a bit and didn't find any bugs. Very nice! I like that the UI prevents "Max Time Unit" from being less than "Min Time Unit".
-
I support the choice of day.js. I looked through the formatting options for day.js vs those for date-fns. Day.js is definitely more limited. For example, I can't format values like
May 5th, 2022
or12:00 noon
with it, whereas I can obtain those formatted results using date-fns. But all-in-all, these limitations seem acceptable. Users will definitely be able to format their date/time data in a manner that looks good and properly conveys the underlying values -- even if they can't tweak it for display exactly as they might want, were they to render the data in some other context like a document. -
I have noted one small change request which I hope is agreeable and easy enough to implement that we can handle it before merging.
@pavish I've just looked over the diff of your most recent changes on my phone and they look good. I think I have properly enabled Auto merge and merged Master into this branch, but it's a little hard to tell on my phone. I might not be at work today, so please feel free to do whatever else is necessary to go PR merged yourself. |
Since @seancolsen has given his approval via comment, I'm going to dismiss the previous review inorder to get this PR merged. |
Fixes #261 , Fixes #1337 , Fixes #1108
Screen.Recording.2022-05-05.at.6.27.28.AM.mov
Note:
mm:ss
and then change format tohh
. In this case, the hour value will be fractional.show_units
display option is removed for the sake of faster completion.dayjs
is installed inorder to be used with duration and for date & time types. The main consideration was the library size anddayjs
is a lot smaller than the other options. Since we not require heavy date time operations on the frontend, this seems like the best choice.Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin