-
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] portal.js #4722
[typescript-migration] portal.js #4722
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.
@Olenka-Yurchuk 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
+ 52
- 36
59% TSX
41% JavaScript
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
Thanks, CI is failing on prettier can you run prettier locally and commit those changes? |
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 update to typescript here looks good. The only callout was already made by another reviewer with the update of the package manager in the package.json and confirming why that would be included in this PR. Once that is confirmed or removed this should be good to go.
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.
package.json
Outdated
@@ -121,5 +121,6 @@ | |||
"prettier --write", | |||
"git add" | |||
] | |||
} | |||
}, | |||
"packageManager": "yarn@1.22.17" |
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.
As for me, this is OK, because w/o this line yarn will use your local version and it could be not a 1.x
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4722 +/- ##
==========================================
+ Coverage 97.16% 97.26% +0.09%
==========================================
Files 18 11 -7
Lines 1765 1461 -304
Branches 742 651 -91
==========================================
- Hits 1715 1421 -294
+ Misses 50 40 -10 ☔ 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.
name: Migrate portal.jsx
Description
Linked issue: #4700
Changes
portal.jsx
was migrated to ts + added JSDocsContribution checklist