Refactor ImportProfileModal to support mod list updates #1598
Conversation
We want to access the preview step ASAP as this is where user is likely to encounter problems with outdated or not loaded mod list.
Some call sites of the Vuex methods pass objects of wrong but compatible type to the methods. TypeScript doesn't pick up on this as the type support for the old Vuex version is non-existent. Extend the argument types so some later changes to methods don't break when the ExportMod object is passed.
The modal now shows a list of the known mods which can be installed, as well as a list of the names of the unknown mods. If there's no known mods, the import button is disabled. Option to cancel the import is always shown, in case user wants to cancel after seeing the contents of the profile. The main reason for combining the modals is to make it easier to provide an option for updating the mod list without leaving the modal. This became important after the recent changes, as the splash screen no longer guarantees that the mod list is loaded into Vuex.
SplashScreen no longer guarantees the online mod list has been loaded into the memory, or the list might be fetched from IndexedDB and be outdated. Profile import can't install mods that are not found in the memory. To remedy the issue, allow users to trigger mod list update directly from the profile import modal.
anttimaki
force-pushed
the
splash-speedrun-pt4
branch
from
349edb6 to
bc97cde
Compare
anttimaki
force-pushed
the
splash-speedrun-pt5
branch
from
6c2348e to
b8a5f50
Compare
| | 'PROFILE_IS_BEING_IMPORTED' | ||
| = 'IMPORT_UPDATE_SELECTION'; | ||
| = 'FILE_CODE_SELECTION'; |
There was a problem hiding this comment.
Not an issue, just curious as to why the order changed? Seems somewhat unnecessary?
There was a problem hiding this comment.
My idea was to fail early. As the step where the import process is most likely to "fail" is the preview step, I wanted to take the user there without having to e.g. create a new profile folder first.
|
|
||
| if (mode === 'UPDATE') { | ||
| this.targetProfileName = this.activeProfileName; | ||
| get profileMods(): {known: ThunderstoreMod[], unknown: string[]} { |
There was a problem hiding this comment.
This would be nicer as an interface IMO. Saves having to redefine the type across files.
There was a problem hiding this comment.
I'm not sure which files you refer to? The type isn't used elsewhere and doesn't need to be redefined even within the modal file.
| @@ -317,32 +306,83 @@ export default class ImportProfileModal extends mixins(ProfilesMixin) { | |||
| <h2 class="modal-title">Packages to be installed</h2> | |||
| </template> | |||
| <template v-slot:body> | |||
| <OnlineModList :paged-mod-list="profileToOnlineMods" :read-only="true" /> | |||
| <OnlineModList :paged-mod-list="profileMods.known" :read-only="true" /> | |||
| <div v-if="!profileMods.known.length || profileMods.unknown.length" class="notification is-warning margin-top"> | |||
There was a problem hiding this comment.
Can the length assertions include the number?
Aware !0 results in false, but I don't think it's overly readable. I'd prefer .length == 0 && ....
Writing for type safety is much nicer instead of relying on the conversion to boolean.
Applies to other v-if blocks too
| Some of the packages in the profile were not found on Thunderstore: | ||
| </p> | ||
|
|
||
| <p class="margin-top">{{ profileMods.unknown.join(", ") }}</p> |
There was a problem hiding this comment.
Worth making computed? Moves logic out of the actual render.
| <p v-else class="margin-top"> | ||
| Ensure the profile is intended for the currently selected game. | ||
| </p> |
There was a problem hiding this comment.
This is probably fairly useless as an indicator since the BepInEx/MelonLoader packages can be shared across games.
Would it be better UX if we have another modal step, and in that step we re-show the game name and ask if it's correct?
There was a problem hiding this comment.
Granted, this was a 20% effort to get a 80% result.
IDK about the separate step. I kinda like the idea of having all the possible problems on the same step, and not having extra steps when no problems are detected, to keep the process simple. That being said, I was wondering if we could add the info about the targeted game into the r2x file? That way we could explicitly detect this issue (for new profiles anyway) and tell the user to change the game.
| </span> | ||
| <span v-else-if="$store.state.tsMods.thunderstoreModListUpdateError"> | ||
| Error updating the mod list: | ||
| {{ $store.state.tsMods.thunderstoreModListUpdateError }}. |
There was a problem hiding this comment.
Do we have a similar concern here as with one of the previous PRs? Should we instead give the user the option to view the error in a separate error modal? Do they actually care about an inline variant of the message?
Generally the error modal is a fixed place for "something went wrong, we're informing you on what it is".
The actual purpose of this part is to allow the user to try again.
There was a problem hiding this comment.
The way I see it is that the user might want to proceed with the import even if the update fails. Opening the error modal would prevent them from continuing where they left off, which to me is more awkward than the inline error text. I'm not completely against changing this though, if it's seen as an improvement.
| class="button is-danger" | ||
| @click="closeModal" | ||
| > | ||
| Cancel import |
There was a problem hiding this comment.
I think Cancel alone is fine? Not overly fussed.
We don't usually offer "Cancel" actions though. It's expected that the user would just click off the modal. Might be nice to make it consistent in other places if we're going to start doing this.
- Explicitly check array lengths in conditional statements in template - Add computed property for unknownProfileModNames - Remove cancel button as they really aren't used elsewhere in the app
|
🤷🏻 = Changes implemented in #1622 |
No description provided.