-
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 DatePickerProps #4932
fix DatePickerProps #4932
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.
@yuki0410-dev 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
+ 43
- 24
100% TSX
Type of change
Fix - These changes are likely to be fixing a bug or issue.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4932 +/- ##
==========================================
- Coverage 96.67% 96.35% -0.32%
==========================================
Files 28 28
Lines 3275 3295 +20
Branches 1357 1392 +35
==========================================
+ Hits 3166 3175 +9
- Misses 109 116 +7
- Partials 0 4 +4 ☔ 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.
The changes look good to me overall. I added one note about possibly simplifying the default value checks, but otherwise don't have any concerns about the updates. Thanks!
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.
I have no concerns with this pull request. The duplication already mentioned would be nice to have but is non-blocking.
Reviewed with ❤️ by PullRequest
it looks like the test coverage took a hit, can you review this? |
@martijnrusschen |
bd5f1e0
to
ee69d7b
Compare
@martijnrusschen |
Description
Linked issue: closes #4922
Problem
See issue
Changes
Pick<AdditionalProps, "excludeScrollbar">
Contribution checklist