-
Notifications
You must be signed in to change notification settings - Fork 176
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
Introduce ProfilesMixin and clean up some of Profiles.vue's code #1359
base: create-profile-modal-refactor
Are you sure you want to change the base?
Introduce ProfilesMixin and clean up some of Profiles.vue's code #1359
Conversation
Draft, no build errors, but modal functionality isn't working. |
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.
Not much to comment at this point. The beef of this PR will be moving the business logic from the component to Vuex
</script> | ||
<template> | ||
<!-- Profile import / update selection modal --> | ||
<ModalCard v-if="isOpen && activeStep === 'IMPORT_UPDATE_SELECTION'" :is-active="isOpen" @close-modal="closeModal"> |
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 consider having just one ModalCard component, and having each step as a <template v-if="activeStep === ...>
. Would be cleaner IMO. That being said, IDK if it will work nicely with what ModalCard expects it's contents to be.
2642140
to
638ee31
Compare
More work done, importing works, updating doesn't, I'll try to fix that next. Didn't move anything to vuex yet. |
638ee31
to
0a2652f
Compare
Finally got the importing working also, up next I'll continue testing/fixing. After testing seems ok, I'll move stuff to vuex and to mixins, After that I'll remove the old code. |
// When updating: | ||
// 1. Reads and parses the mods to be installed | ||
// 2. Creates an EventListener which gets triggered after the user has selected the old profile to be updated. The callback then: | ||
// 1. Creates a temporary profile (called _profile_update) |
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.
Actually to me it seems more like:
- Deletes the temporary profile (called _profile_update) if it exists (leftover from previous failed run or something like that?)
- Downloads (and extracts) the mods to temporary profile
- Deletes the old profile (including the folder on disk)
- Renames temporary profile to the chosen profile's name
Which makes more sense, since I assume the point of having the temp folder is to not lose the original profile if something goes wrong when downloading/extracting the mods.
351460c
to
a7fa19f
Compare
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.
- Please split this into two separate PRs
- The first one should introduce the mixin and move the methods from the modal components into the mixin
- I was thinking that the mixin would be for the modals, not for the Profiles.vue. It might make sense to use the mixin for Profiles.vue too, but it's hard for me to judge it currently due to the scope of this PR
- The second PR should be about separating the import modal from Profiles
- This can add additional methods to the mixin if it makes sense
6cd313b
to
e0d1e5a
Compare
return sanitize(nameToSanitize); | ||
} | ||
|
||
async setSelectedProfile(profileName: string, prewarmCache = true) { |
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.
As far as I can tell, even when checking the next PR in the stack, this method and all below it in this file, are only used by Profiles.vue. If this is the case, what is the rationale of moving them into the mixin? Seems we would get the cons (having the code split into multiple places) without the pros (code reusability).
This also applies to appName
getter.
e0d1e5a
to
97a1264
Compare
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
No description provided.