Refactor Rename Profile Modal by separating it from Profiles.vue #1328
Conversation
VilppeRiskidev
force-pushed
the
delete-profile-modal-refactor
branch
from
7889a82
to
99cbcba
Compare
VilppeRiskidev
force-pushed
the
rename-profile-modal-refactor
branch
2 times, most recently
from
d502c3b
to
211998f
Compare
VilppeRiskidev
force-pushed
the
delete-profile-modal-refactor
branch
from
6ec0a39
to
5794d5d
Compare
VilppeRiskidev
force-pushed
the
rename-profile-modal-refactor
branch
from
211998f
to
a352dbc
Compare
There was a problem hiding this comment.
Quite many comments but mostly minor ones. Also they're all new ones, so good job picking up the lessons from the previous PR!
Bonus cookie points if you can make the the input submit the changes by pressing enter instead of having to click the submit button with mouse.
| <template v-slot:body> | ||
| <p>This profile will store its own mods independently from other profiles.</p> | ||
|
|
||
| <input class="input" v-model="newProfileName" ref="profileNameInput" /> |
There was a problem hiding this comment.
The ref attribute was part of these changes and most likely shouldn't be present in this standalone component. Would HTML's autofocus attribute work here?
There was a problem hiding this comment.
doesn't work for some reason, I guess it'd have to be a <dialog>
VilppeRiskidev
force-pushed
the
rename-profile-modal-refactor
branch
from
a352dbc
to
0266d09
Compare
VilppeRiskidev
force-pushed
the
delete-profile-modal-refactor
branch
from
cd8c2ff
to
2faa289
Compare
VilppeRiskidev
force-pushed
the
rename-profile-modal-refactor
branch
from
0266d09
to
c5648ab
Compare
| components: {ModalCard} | ||
| }) | ||
| export default class RenameProfileModal extends Vue { | ||
| private newProfileName: string = ''; |
There was a problem hiding this comment.
This still has a bug, where the newprofilename is empty when the modal is first opened (for example coming from the game selection). Doing private newProfileName: string = this.$store.state.profile.activeProfile.getProfileName(); doesn't work either. I'll look into this, but if you can immediately think of a solution feel free to comment.
- `RenameProfileModal` has been separated from `Profiles.vue` - Actions `renameProfile` and `updateProfileList` have been added to `ProfilesModule` - Old profile renaming functionality has been removed from `Profiles` page
The previous implementation didn't take into account that the local value is empty when user enters the Profiles screen, but an "old" profile with a different name might be already selected if user did so in another game's profile selection screen. I think the proper solution would be to ensure that the active profile is reset to null when user returns to game selection screen, but that's a bigger change with a risk of breaking other stuff, so it's not implemented now.
Autofocus is blocked if the URL contains fragments, which it does since vue-router, like many SPA routers, use them to differentiate between the pages. Quick googling would indicate that this problem has been fixed a few years back by WHATWG altering the HTML specification and Chromium implementing the changes. However, in our case the issue seems to persists. But no worries, we can achieve the same result with a few lines of Vue code.
Also disable the rename button to indicate it does nothing if a profile with the chosen name already exists.
anttimaki
force-pushed
the
rename-profile-modal-refactor
branch
from
c5648ab
to
64c0a36
Compare
There was a problem hiding this comment.
@VilppeRiskidev: Uno reverse card! I'll mark the PR approved but I expect you to review it after the changes I've done:
- The PR had merge conflicts with
delete-profile-modal-refactor. It almost seemed like you'd manually reproduced few of the commits from the parent branch (but missed one tiny bit). Anyways, don't do that since it makes reviewing difficult. Use git's rebase instead. - Addressed the few comments that I left unresolved by adding new commits. Check them out (commit messages too) to see how I solved them
- Added a few other fixes, check them out too
- Please test this thoroughly, I was rushing the last changes a bit since this took longer than I anticipated 😅
|
LGTM 👍 |
RenameProfileModalhas been separated fromProfiles.vuerenameProfileandupdateProfileListhave been added toProfilesModuleProfilespage