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.

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

Refactor cancel dialog confirmation check to service #1135

Conversation

vigneshsankariyer1234567890
Copy link
Contributor

@vigneshsankariyer1234567890 vigneshsankariyer1234567890 commented Jan 18, 2023

Summary:

Fixes #1024

Changes Made:

  • Refactoring the check to open or cancel the Cancel Dialog if there have been any changes made to the issue to IssueService
  • Originally, there was a lot of code duplication to even check issue was modified. Instead, this was refactored to the IssueService class to reduce such code duplication.

Proposed Commit Message:

Refactor Cancel Dialog Confirmation check to service

Copy link
Contributor

@kkangs0226 kkangs0226 left a comment

Choose a reason for hiding this comment

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

Thanks for your work! I have left a few comments for you.

Also, seems like there is an existing PR for the same issue, and because it has been made first, we should merge that PR first 😅 Thanks for looking into this regardless, I hope you are more familiar with our codebase now.

* An interface that provides abstraction for components that can cancel
* edits.
*/
export interface EditCancellable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid exporting other interfaces/classes in service files.

* @param component Component that implements `EditCancellable`
* @param isModified Callback function that checks if the field has actually been modified
*/
openCancelDialogIfModified<T extends EditCancellable>(component: T, isModified: () => boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the start of the file, you will see that the IResponsible for creating and updating issues, and periodically fetching issues using GitHub. It would be better to have this method under another file.

@vigneshsankariyer1234567890
Copy link
Contributor Author

Closing this PR since work has already been done on the issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce code duplication in bug reporting, team response and tester response views
2 participants