-
Notifications
You must be signed in to change notification settings - Fork 68
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
Refactor cancel dialog confirmation check to service #1135
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.
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 { |
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.
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) { |
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.
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.
Closing this PR since work has already been done on the issue :) |
Summary:
Fixes #1024
Changes Made:
IssueService
IssueService
class to reduce such code duplication.Proposed Commit Message: