-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add support second-level granularity to injectTimes #4683
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
Conversation
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 pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@yykcool you can click here to see the review status or cancel the code review job.
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.
PullRequest Breakdown
Reviewable lines of change
+ 54
- 11
51% JavaScript (tests)
48% JavaScript
2% Other
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
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.
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 PR looks good to me overall for adding support for seconds. I had two comments for minor adjustments that need to be made below, but overall I didn't notice any major issues here.
Thanks for taking care of this!
Reviewed with ❤️ by PullRequest
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.
Please fix the test case comments
comments are addressed |
Can you make sure to merge with the latest main branch to upload the code coverage file? |
it seems like things are already up to date (tried to do a merge from the main branch), is there anything i might have missed? |
can you try again? |
done |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4683 +/- ##
=======================================
Coverage 97.00% 97.00%
=======================================
Files 28 28
Lines 2635 2638 +3
Branches 1133 1116 -17
=======================================
+ Hits 2556 2559 +3
Misses 79 79 ☔ View full report in Codecov by Sentry. |
Description
Linked issue: None
Problem
seconds from Datetime objects passed into injectTime setting is currently ignored by the date picker
Changes
added support for seconds, changed doc-site example to reflect it, and added 1 test as well.
also corrected
timeIntervals
tointervals
on existing tests found ininject_times_test.test.js
to account for the fact thattimeIntervals
does not exist as a parameter, and the correct one should beintervals
insteadContribution checklist