-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fix: default time selection #4552
Fix: default time selection #4552
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.
@kiselevskym 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
+ 62
- 0
79% JavaScript (tests)
21% JavaScript
Type of change
Fix - These changes are likely to be fixing a bug or issue.
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.
The change looks good to me with a minTIme
support in time selection
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 make sure to add a test case to check if the min time gets set |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4552 +/- ##
=======================================
Coverage 95.51% 95.52%
=======================================
Files 29 29
Lines 2566 2569 +3
Branches 1053 1055 +2
=======================================
+ Hits 2451 2454 +3
Misses 115 115 ☔ View full report in Codecov by Sentry. |
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 reviewed the updates made to #4552 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.
Reviewed by:
Test cases were added |
test/min_time_test.test.js
Outdated
@@ -0,0 +1,52 @@ | |||
import React, { useState } from "react"; | |||
import DatePicker from "../src/index.jsx"; | |||
import { mount } from "enzyme"; |
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.
Can you avoid using enzyme? You can use React Testing Library.
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.
no prob, can you check it now?
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.
showTimeSelect
andminTIme
.Actual Result (AR):
Despite disabling the start time, the Datepicker defaults to selecting the beginning of the day.
Expected Result (ER):
The Datepicker should select the minimum allowable time upon choosing a day.