[typescript-migration] month.jsx #4743
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4743 +/- ##
==========================================
- Coverage 97.26% 96.85% -0.42%
==========================================
Files 11 10 -1
Lines 1462 1143 -319
Branches 642 513 -129
==========================================
- Hits 1422 1107 -315
+ Misses 40 36 -4 ☔ View full report in Codecov by Sentry. |
|
@martijnrusschen hello, it looks like pullrequest stack, because I don't see it as a reviewer |
|
apologies, I'm on vacation this week and don't have the ability to review
or merge anything until Friday.
…On Mon, Apr 29, 2024 at 2:58 PM Mirus ***@***.***> wrote:
@martijnrusschen <https://github.com/martijnrusschen> hello, it looks
like pullrequest <https://github.com/apps/pullrequest> stack, because I
don't see it as a reviewer
—
Reply to this email directly, view it on GitHub
<#4743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKY2KBYTAKSD3IIIHGMXODY7Y7W5AVCNFSM6AAAAABG52O446VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBSGY2TSMRVGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ 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.
@mirus-ua you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
PullRequest Breakdown
Reviewable lines of change
+ 423
- 226
97% TSX
3% TypeScriptType of change
Feature - These changes are adding a new feature or improvement to existing code.
| 1 Message | |
|---|---|
| Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers. |
There was a problem hiding this comment.
this looks like a fairly simple change to do things in a more TS way. Are there any unit tests for this that need to be modified as well?
Reviewed with ❤️ by PullRequest
| @@ -845,8 +845,7 @@ export function isMonthDisabled( | |||
| minDate: minDate ? startOfMonth(minDate) : undefined, | |||
| maxDate: maxDate ? endOfMonth(maxDate) : undefined, | |||
| }) || | |||
| (excludeDates && | |||
| excludeDates.some((excludeDate) => isSameMonth(month, excludeDate))) || | |||
| excludeDates?.some((excludeDate) => isSameMonth(month, excludeDate)) || | |||
There was a problem hiding this comment.
I believe this could introduce a functional change and should be verified carefully. The original statement includes the logical AND operator (&&) to first check if excludeDates is truthy (i.e., not null, undefined, false, 0, NaN, or an empty string). The modified statement uses the optional chaining operator (?.) to call .some() on excludeDates only if excludeDates is not null or undefined.
The key difference here is the return value when excludeDates is null or undefined: it is undefined rather than false as in the previous case. This distinction could affect how the expression behaves in a broader logic context where undefined and false might be handled differently.
There was a problem hiding this comment.
I don't think someone considered false values in this case. Moreover, our types clearly say that we have only two options: Date[] or undefined.
Last but not least, all tests are green, and right now, this is a source of truth w/o other documentation and specs.
There was a problem hiding this comment.
I have marked this as approved, but want to request you review the comments on the potential behavior change in excludeDates to ensure the functionality is not changing unexpectedly. Thank you!
Reviewed with ❤️ by PullRequest
| @@ -942,8 +938,7 @@ export function isYearDisabled( | |||
| minDate: minDate ? startOfYear(minDate) : undefined, | |||
| maxDate: maxDate ? endOfYear(maxDate) : undefined, | |||
| }) || | |||
| (excludeDates && | |||
| excludeDates.some((excludeDate) => isSameYear(date, excludeDate))) || | |||
| excludeDates?.some((excludeDate) => isSameYear(date, excludeDate)) || | |||
| (includeDates && | |||
There was a problem hiding this comment.
I didn't touch includeDates because of !. In this case is hard to separate missing values and some method result revertion.
includeDates &&
!includeDates.some((includeDate) => isSameYear(date, includeDate)))| @@ -1504,7 +1499,9 @@ | |||
| * @param event - The keyboard event. | |||
| * @returns - Returns true if the space key was pressed down, false otherwise. | |||
| */ | |||
| export function isSpaceKeyDown(event: KeyboardEvent): boolean { | |||
| export function isSpaceKeyDown( | |||
| event: React.KeyboardEvent<HTMLDivElement>, | |||
name: Migrate month.jsx
Description
Linked issue: #4700
Changes
month.jsxwas migrated to ts + added JSDocsContribution checklist