-
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] time.jsx #4726
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
+ 81
- 63
99% TSX
1% JavaScript (tests)
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
ac50a6a
to
730c0fd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4726 +/- ##
==========================================
+ Coverage 96.85% 97.28% +0.43%
==========================================
Files 13 12 -1
Lines 1589 1476 -113
Branches 706 652 -54
==========================================
- Hits 1539 1436 -103
+ 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.
Overall the code change looks good and matches the description regarding migration to TS. I just provided some minor comments.
Reviewed with ❤️ by PullRequest
b68d4a4
to
c349829
Compare
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.
Only one nit regarding a return type not being specified. Other than that, I see no issues.
Reviewed with ❤️ by PullRequest
src/time.tsx
Outdated
private list?: HTMLUListElement; | ||
|
||
private centerLi?: HTMLLIElement; | ||
|
||
componentDidMount() { |
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.
Corrected.
@mirus-ua
However, this should be detected by ESLint in CI, so the lint rule may need to be modified.
c349829
to
1bc5ecf
Compare
1cbdc0d
to
1b29b33
Compare
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.
1b29b33
to
f6b886d
Compare
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 time.jsx
Description
Linked issue: #4700
Changes
time.jsx
was migrated to tsContribution checklist