-
Notifications
You must be signed in to change notification settings - Fork 148
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
[MM-199] Convert files related to redux from js to ts #743
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #743 +/- ##
=======================================
Coverage 16.16% 16.16%
=======================================
Files 17 17
Lines 6021 6021
=======================================
Hits 973 973
Misses 5003 5003
Partials 45 45 ☔ 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.
LGTM, just a few comments
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.
LGTM
@mickmister Gentle reminder to re-review the PR. |
webapp/src/types/github_types.d.ts
Outdated
user: User; | ||
} | ||
|
||
type Item = PrsDetailsData & { |
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.
I would prefer to have these types exported, and explicitly imported elsewhere. Only ones that are imported elsewhere need to be exported.
Also Item
is a pretty vague name too, so having these global like this is def a bit cryptic. Honestly not sure what Item
this is referring to. Would FullPrDetails
be appropriate? Or does this also apply to Issues
as well?
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.
Updated the name to GithubItem
. Let me know if this is fine or if I should update the name.
@mickmister fixed the review comments. Please re-review. |
}; | ||
|
||
function sidebarContent(state = defaultSidebarContent, action) { | ||
} as SidebarContentData, action: {type: string, data: SidebarContentData}) { |
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.
I like this as
usage here 👍
@AayushChaudhary0001 Gentle reminder to QA to review the changes. |
@hanzei Will test it by end of this week, if this is not of high priority. Please let me know if this needs to be tested urgently. |
End of week is fine, thanks @AayushChaudhary0001 |
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 PR has been tested for all the basic functionalities, subscriptions and slash commands. This PR is working fine in all the conditions, LGTM. Approved.
@mickmister Should we merge the above PR and start creating HW (typescript migration) tickets? |
@ayusht2810 Yeah merged 👍 |
@ayusht2810 Before creating the tickets, I think we should have one example PR that shows how to convert a given component (and |
Summary
Ticket Link
NA
What to test?