feat: move user settings to multiple components #889
No reviewers
dpschen
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
2 Participants
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#889
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature/user-settings"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR Moves all user settings to separate pages to declutter them and make it easier for users to quickly find what they're looking for.
Here's how it looks:
👋 I hope the comments don't sound too rude / harsh.
Realized that after finishing everthing and don't want to rewrite again.
See them all together as ideas for improvements / personal views that can happen in future or never :)
@ -288,2 +2,2 @@</p></card><div class="user-settings"><ul>Wrap ul in
<nav>elementDone!
@ -415,1 +64,3 @@}computed: mapState({totpEnabled: state => state.config.totpEnabled,migratorsEnabled: state => state.config.availableMigrators !== null && state.config.availableMigrators.length > 0,How about making
migratorsEnableda getter in the store?The rest can be simplified with:
Done (both of it).
@ -418,0 +73,4 @@.user-settings {display: flex;max-width: $widescreen;margin: 0 auto;If we put the margin in a wrapper component, this component can be used in a different context without change, e.g. a modal.
As a rule of thumb: never add outer margin / positioning to a component wrapper element.
You mean wrapping the
<router-view>this is mounted in with amargin: 0 auto?No. I guess that also doesn't make sense.
My idea was
Reason is that it make components self enclosed and don't have leaking style that prevents them (generally) from other future forms of usage. In this case it means that this component prevents itself from beeing used in a use case where we don't want the centering margin.
Maybe a simpler and easier example would be:
Never add
position: fixedto the outer div of a component but instead always do that from the outside.Because then you can use that component also in cases where it's not fixed.
Sum up: don't include the outer positioning of a component inside.
All that makes it much easier to maintain a design system.
I see, thanks for clarifying. Sounds very reasonable.
I've added a wrapper div in the component, hope that clears it up.
Sounds good!
I think abstracting this to another component just makes sense when we have a usecase somewhere else =)
@ -418,0 +75,4 @@max-width: $widescreen;margin: 0 auto;ul {ul, andli adon't have to be nested.Better would be to add classes to the elements because they make maintaining the CSS simpler by:
Done.
@ -0,0 +66,4 @@import {Cropper} from 'vue-advanced-cropper'import 'vue-advanced-cropper/dist/style.css'import AvatarService from '../../../services/avatar'Use
@alias so that we don't have to rewrite import paths every time we move a component to a new folderYeah that's something I need to teach webstorm so it doesn't mess it up all the time.
Adjusted in all new components.
@ -0,0 +1,58 @@<template><card v-if="caldavEnabled" :title="$t('user.settings.caldav.title')">This would show a empty view if caldav gets disabled out of whatever reason.
How about using the
beforeRouteEnterhook, redirect to the general settings and throwing an error?How would I access the config in state?
thisdoesn't seem to be available in the before hook.You can import the store directly =)
EDIT:
Btw, another advantage of using the store directly in these cases is:
You can make the guards reusable and import them in multiple components. Allthough I guess in this case we don't need that.
But couldn't that lead to issues where the store isn't pouplated already when calling it here? The only way to ever trigger that hook is by manually entering it into the browser's address bar which then boots everything and populates the store (and also does a call to the config api endpoint to figure out if caldav is enabled or not). That means it wouldn't even work in the one case where it was called. In all other cases, there's just no link to that page that could trigger it.
Solving this with new issue: #906
@ -0,0 +17,4 @@</div></div><p><a href="https://vikunja.io/docs/caldav/" rel="noreferrer noopener nofollow" target="_blank">The url should be a const that is defined at the same place as the link behind "powered by Vikunja".
As a minimum maybe inside this document but outside the default export so that it can be combined with the latter in a later step easily.
Makes sense. Done.
@ -0,0 +35,4 @@},computed: {caldavUrl() {let apiBase = window.API_URL.replace('/api/v1', '')This feels like a hack.
The apiBase should be the one that gets defined in the initial api-config.vue
Regardless of the above: this url manipulation is a great case for https://github.com/unjs/ufo/ that we already use in the
api-config.vueHow about moving this to the store as a getter. This way this component has nothing to do with the api configuration and the included hack.
That really is a hack from ancient times...
Moved it to a getter.
@ -0,0 +45,4 @@return `${apiBase}/dav/principals/${this.userInfo.username}/`},...mapState({See comment above regariding usage of
mapStateEven though the two variables from state I'm using here come from different components?
you need to use it then twice of course. I just thought it's less verbose because you just need two lines and don't have all that duplication (
caldavEnabledonce as prop in the store and once as the local representation — we want it to be the same either way).I've change the config one but kept the other one. I wanted to keep having
auth.infoasuserinfoinstead of justinfowhich it would have been if I'd just usemapState('auth', ['info']). Hope that makes sense.@ -0,0 +52,4 @@this.setTitle(`${this.$t('user.settings.updateEmailTitle')} - ${this.$t('user.settings.title')}`)},methods: {async updateEmail() {I think we should move methods like these in the store as actions.
This makes it possible to do stuff like updating the email from other locations easily.
Make that's not the best example but for other settings this would help us reduce a lot of repetitive code.
I'm not sure if you should be able to change the email from somwhere else than this component. What other settings functions do you have in mind?
For example could this be useful to make the changes directly but add a notification with an undo action. The undo action triggers that function in the store with the cached old state.
Another idea: resetting email (not sure if this is really the same logic, didn't check)
It's the idea of keeping bussiness logic separated from the view logic. That makes it easy to access the same logic from different components.
If it's just the undo action, you could probably get away with just using an arrow function. (that actually works already, see here).
I do get your point though. Shouldn't I then replace this everywhere? How do I decide what goes directly into a component and what doesn't?
@ -0,0 +6,4 @@<input@keyup.enter="updateSettings"class="input"id="newName"The id should include a random id to prevent future errors of duplicate ids.
Done.
@ -0,0 +44,4 @@</div><div class="field"><label class="checkbox"><input type="checkbox" v-model="playSoundWhenDone"/>When this gets changed to
truewe should play the actual sound.@ -0,0 +132,4 @@availableLanguages() {return Object.entries(availableLanguages).map(l => ({code: l[0], title: l[1]})).sort((a, b) => a.title > b.title)We should use localCompare for sorting by abc
@ -0,0 +4,4 @@<div class="field"><label class="label" for="newPassword">{{ $t('user.settings.newPassword') }}</label><div class="control"><inputWe should add the correct autocomplete attributes so that the changed login data is easier available in password managers.
@ -0,0 +1,130 @@<template><card :title="$t('user.settings.totp.title')" v-if="totpEnabled">Same here with the
beforeRouteEnter.See new issue: #906
@ -0,0 +19,4 @@<div class="field"><label class="label" for="totpConfirmPasscode">{{ $t('user.settings.totp.passcode') }}</label><div class="control"><inputAdd
autocomplete="one-time-code"@ -0,0 +35,4 @@{{ $t('user.settings.totp.setupSuccess') }}</p><p v-if="!totpDisableForm"><x-button @click="totpDisableForm = true" class="is-danger">{{ $t('misc.disable') }}</x-button>This should be a toggle button so that a curious user can go back in the previous ui state.
But the button is not visible when
totpDisableFormis true. It requires a different action to disable it.What I meant is: UI should be reversible.
See https://www.interaction-design.org/literature/article/shneiderman-s-eight-golden-rules-will-help-you-design-better-interfaces
under "Permit easy reversal of actions".
If a user clicks that button to enable that form, there should be a way to go back to the old state where the form was collapsed without the need to reload.
My idea was to keep that button and make a toggle button out of it.
As an alternative there could be a close button (?). But since it's some kind of collapsable I thought the toggle button makes sense here.
Good point. I've added a "cancel" button next to the "disable" button which reverses the UI state. I think that should be enough.
@ -0,0 +106,4 @@},async totpSetQrCode() {const qr = await this.totpService.qrcode()const urlCreator = window.URL || window.webkitURLRemove
window.webkitURL. In other places in the codebasewindow.URL.createObjectURLis called directly.27d1b2f301to3ff2c99a35@ -36,2 +36,4 @@},}),getters: {migratorsEnabled: state => state.availableMigrators !== null && state.availableMigrators.length > 0,use
state.availableMigrators?.length > 0Ready to merge?