-
Notifications
You must be signed in to change notification settings - Fork 196
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
Download callback refactor #1594
base: develop
Are you sure you want to change the base?
Conversation
569946e
to
4d80eb4
Compare
I based this branch on the previous branches since those have been approved already and are just waiting to be merged. |
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.
The changes look good 👍🏻 I still found some reasons to flap my gums so read the commits for future reference.
@@ -56,6 +58,22 @@ async function extractConfigsToImportedProfile( | |||
} | |||
} | |||
|
|||
export async function installModsAndResolveConflicts( |
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.
Food for a thought, not a change request:
Given that this is in the end a rather simple piece of code, it could be argued that having it in the utils is a bit of overkill, especially since we have another wrapper in the mixin that reduces duplication. Another thing that IMO speaks a bit against having it here is the fact that we need to pass the store as an argument. That's not inherently bad, but a weak signal that the split between UI component and helper library should be considered. For example, this helper is now not reusable if the call site doesn't have access to the store. Ideally we could import the store singleton to helpers but I think that's currently unnecessarily difficult due to R2MM & TSMM having different store definitions.
I guess that my point is that I've no strong arguments one way or another, but my nitpick level opinion is that passing the store as an argument should be avoided if it's easy to do so.
}, this.downloadCompletedCallback); | ||
}, (downloadedMods) => { | ||
this.downloadCompletedCallback(downloadedMods); | ||
this.downloadingMod = false; |
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.
Just mentioning this in the hopes we remember to do this in the future PRs: this should be something like isDownloadProgressModalOpen
and probably to be handled into ModalModule
store. Ideally in a way that when one of the three modals in this file is opened, the other two are automatically closed.
4d80eb4
to
cc472eb
Compare
split DownloadModModal component
6ac4f07
to
93703e6
Compare
Rebased |
No description provided.