-
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
[typescript-migration] initial ts config and the first calendar_container.jsx module #4704
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.
@mirus-ua 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
+ 61
- 18
34% JSON with Comments
23% JavaScript
22% JSON
22% TSX
Generated lines of change
+ 49
- 3
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4704 +/- ##
==========================================
- Coverage 97.13% 97.12% -0.01%
==========================================
Files 28 27 -1
Lines 2685 2677 -8
Branches 1147 1142 -5
==========================================
- Hits 2608 2600 -8
Misses 77 77 ☔ 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.
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.
Love to see a typescript migration! I left one comment around the naming of the build script in package.json
.
I would also recommend making sure that calendar_container.tsx
is showing up correctly in the dist
folder when building for production to verify that this setup is working correctly.
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.
@martijnrusschen hello. I'm not sure that I can increase the test coverage because the PR is mostly about configs 😅 |
Nice one! Glad to see this coming along! |
name: initial ts config and the first calendar_container.jsx module
Description
Linked issue: #4700
Problem
Changes
Was added initial ts config and rollup plugins + migrated the first functional component to TS
To reviewers:
I've added babel preset-typescript instead of using ts-jest because right now, during the migration, better to only transpile code and not check types validity in tests
Screenshots
Contribution checklist