Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[MM-199] Convert files related to redux from js to ts #743

Merged
merged 6 commits into from
May 14, 2024
Merged

[MM-199] Convert files related to redux from js to ts #743

merged 6 commits into from
May 14, 2024

Conversation

ayusht2810
Copy link
Contributor

Summary

  • Converted action, reducers, selectors, and action_types files from JS to TS.
  • Added new folder to contain the types used in above files.

Ticket Link

NA

What to test?

  • Complete testing of the plugin

@ayusht2810 ayusht2810 self-assigned this Feb 13, 2024
@ayusht2810 ayusht2810 added the 2: Dev Review Requires review by a core committer label Feb 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.16%. Comparing base (6cc3429) to head (7b58b12).
Report is 5 commits behind head on master.

❗ Current head 7b58b12 differs from pull request most recent head b9e4833. Consider uploading reports for the commit b9e4833 to get more accurate results

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.
📢 Have feedback on the report? Share it here.

@hanzei hanzei removed their request for review February 13, 2024 12:28
mickmister
mickmister reviewed Mar 14, 2024
View reviewed changes
Copy link
Contributor

@mickmister mickmister left a 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

webapp/src/actions/index.ts Outdated Show resolved Hide resolved
webapp/src/types/index.d.ts Outdated Show resolved Hide resolved
webapp/src/selectors.ts Outdated Show resolved Hide resolved
webapp/src/types/store.d.ts Outdated Show resolved Hide resolved
raghavaggarwal2308
raghavaggarwal2308 approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@raghavaggarwal2308 raghavaggarwal2308 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ayusht2810 ayusht2810 added this to the v2.3.0 milestone Apr 5, 2024
@ayusht2810
Copy link
Contributor Author

@mickmister Gentle reminder to re-review the PR.

mickmister
mickmister approved these changes Apr 22, 2024
View reviewed changes
webapp/src/types/github_types.d.ts Outdated
user: User;
}

type Item = PrsDetailsData & {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

webapp/src/types/github_types.d.ts Outdated Show resolved Hide resolved
webapp/src/selectors.ts Outdated Show resolved Hide resolved
@mickmister mickmister added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Apr 22, 2024
@ayusht2810
Copy link
Contributor Author

@mickmister fixed the review comments. Please re-review.

mickmister
mickmister approved these changes Apr 25, 2024
View reviewed changes
webapp/src/reducers/index.ts
};

function sidebarContent(state = defaultSidebarContent, action) {
} as SidebarContentData, action: {type: string, data: SidebarContentData}) {
Copy link
Contributor

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 👍

@hanzei
Copy link
Contributor

hanzei commented Apr 30, 2024

@AayushChaudhary0001 Gentle reminder to QA to review the changes.

@AayushChaudhary0001
Copy link

AayushChaudhary0001 commented May 1, 2024 •
edited
Loading

@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.

@hanzei
Copy link
Contributor

hanzei commented May 1, 2024

End of week is fine, thanks @AayushChaudhary0001

AayushChaudhary0001
AayushChaudhary0001 approved these changes May 6, 2024
View reviewed changes
Copy link

@AayushChaudhary0001 AayushChaudhary0001 left a 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.

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels May 6, 2024
@ayusht2810
Copy link
Contributor Author

@mickmister Should we merge the above PR and start creating HW (typescript migration) tickets?

@mickmister mickmister merged commit 936e5c3 into master May 14, 2024
9 checks passed
@mickmister mickmister deleted the MM-199 branch May 14, 2024 21:16
@mickmister
Copy link
Contributor

@ayusht2810 Yeah merged 👍

@mickmister
Copy link
Contributor

@ayusht2810 Before creating the tickets, I think we should have one example PR that shows how to convert a given component (and index.ts file in the case of this project), to show how a given typescript migration can be done, and also to validate the current state that it is ready for components to be migrated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kshitij-Katiyar Kshitij-Katiyar Kshitij-Katiyar left review comments

@AayushChaudhary0001 AayushChaudhary0001 AayushChaudhary0001 approved these changes

@raghavaggarwal2308 raghavaggarwal2308 raghavaggarwal2308 approved these changes

@mickmister mickmister mickmister approved these changes

Assignees

@ayusht2810 ayusht2810

Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v2.3.0
Development

Successfully merging this pull request may close these issues.
7 participants
@ayusht2810 @codecov-commenter @hanzei @AayushChaudhary0001 @mickmister @raghavaggarwal2308 @Kshitij-Katiyar