-
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] with_floating.jsx #4709
[typescript-migration] with_floating.jsx #4709
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
+ 67
- 62
52% TSX
48% JavaScript
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 #4709 +/- ##
==========================================
- Coverage 97.10% 97.09% -0.02%
==========================================
Files 26 25 -1
Lines 2663 2651 -12
Branches 1138 1119 -19
==========================================
- Hits 2586 2574 -12
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.
One nit regarding more modern functionality/syntax in JS/TS. If you'd rather I not nitpick about things only tangentially related to the PR, let me know for future reference 😃
Reviewed with ❤️ by PullRequest
src/with_floating.tsx
Outdated
const WithFloating: React.FC<T & WithFloatingProps> = (props) => { | ||
const alt_props = { | ||
...props, | ||
popperModifiers: props.popperModifiers || [], |
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.
Yes, I know about the syntax. I try not to touch anything until it's necessary to satisfy typescript because fixing all outdated patterns and approaches will take much more time than I want to spend on each module, and it may cause unexpected bugs
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.
but I improved this line 316b41f
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 with_floating.jsx
Description
Linked issue: #4700
Changes
with_floating.jsx
was migrated to ts + added JSDocsContribution checklist