vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feat: move user settings to multiple components #889

Merged
konrad merged 27 commits from feature/user-settings into main 2021-10-26 20:58:03 +00:00
Conversation 1 Commits 27 Files Changed 16 +703 -477
16 changed files with 703 additions and 477 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
cypress/integration/user/settings.spec.js
View File

@ -8,7 +8,7 @@ describe('User Settings', () => {
})
it('Changes the user avatar', () => {
cy.visit('/user/settings')
cy.visit('/user/settings/avatar')
cy.get('input[name=avatarProvider][value=upload]')
.click()
@ -28,9 +28,10 @@ describe('User Settings', () => {
})
it('Updates the name', () => {
cy.visit('/user/settings')
cy.visit('/user/settings/general')
cy.get('input#newName')
cy.get('.general-settings .control input.input')
.first()
.type('Lorem Ipsum')
cy.get('.card.general-settings .button.is-primary')
.contains('Save')

4
src/components/home/navigation.vue
View File

@ -147,7 +147,7 @@
</div>
</template>
</aside>
<a class="menu-bottom-link" href="https://vikunja.io" target="_blank" rel="noreferrer noopener nofollow">
<a class="menu-bottom-link" :href="poweredByUrl" target="_blank" rel="noreferrer noopener nofollow">
{{ $t('misc.poweredBy') }}
</a>
</div>
@ -160,6 +160,7 @@ import ListSettingsDropdown from '@/components/list/list-settings-dropdown.vue'
import NamespaceSettingsDropdown from '@/components/namespace/namespace-settings-dropdown.vue'
import draggable from 'vuedraggable'
import {calculateItemPosition} from '@/helpers/calculateItemPosition'
import {POWERED_BY} from '@/urls'
import logoUrl from '@/assets/logo-full.svg'
@ -175,6 +176,7 @@ export default {
},
listUpdating: {},
logoUrl,
poweredByUrl: POWERED_BY,
}
},
components: {

50
src/router/index.js
View File

@ -48,6 +48,14 @@ import CreateSavedFilter from '../views/filters/CreateSavedFilter'
const PasswordResetComponent = () => import('../views/user/PasswordReset')
const GetPasswordResetComponent = () => import('../views/user/RequestPasswordReset')
const UserSettingsComponent = () => import('../views/user/Settings')
const UserSettingsAvatarComponent = () => import('../views/user/settings/Avatar')
const UserSettingsCaldavComponent = () => import('../views/user/settings/Caldav')
const UserSettingsDataExportComponent = () => import('../views/user/settings/DataExport')
const UserSettingsDeletionComponent = () => import('../views/user/settings/Deletion')
const UserSettingsEmailUpdateComponent = () => import('../views/user/settings/EmailUpdate')
const UserSettingsGeneralComponent = () => import('../views/user/settings/General')
const UserSettingsPasswordUpdateComponent = () => import('../views/user/settings/PasswordUpdate')
const UserSettingsTOTPComponent = () => import('../views/user/settings/TOTP')
// List Handling
const NewListComponent = () => import('../views/list/NewList')
@ -115,6 +123,48 @@ const router = createRouter({
path: '/user/settings',
name: 'user.settings',
component: UserSettingsComponent,
children: [
{
path: '/user/settings/avatar',
name: 'user.settings.avatar',
component: UserSettingsAvatarComponent,
},
{
path: '/user/settings/caldav',
name: 'user.settings.caldav',
component: UserSettingsCaldavComponent,
},
{
path: '/user/settings/data-export',
name: 'user.settings.data-export',
component: UserSettingsDataExportComponent,
},
{
path: '/user/settings/deletion',
name: 'user.settings.deletion',
component: UserSettingsDeletionComponent,
},
{
path: '/user/settings/email-update',
name: 'user.settings.email-update',
component: UserSettingsEmailUpdateComponent,
},
{
path: '/user/settings/general',
name: 'user.settings.general',
component: UserSettingsGeneralComponent,
},
{
path: '/user/settings/password-update',
name: 'user.settings.password-update',
component: UserSettingsPasswordUpdateComponent,
},
{
path: '/user/settings/totp',
name: 'user.settings.totp',
component: UserSettingsTOTPComponent,
},
],
},
{
path: '/user/export/download',

10
src/store/modules/config.js
View File

@ -2,6 +2,7 @@ import {CONFIG} from '../mutation-types'
import {HTTPFactory} from '@/http-common'
import {objectToCamelCase} from '@/helpers/case'
import {redirectToProvider} from '../../helpers/redirectToProvider'
import {parseURL} from 'ufo'
export default {
namespaced: true,
@ -35,6 +36,13 @@ export default {
},
},
}),
getters: {
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-25 19:50:00 +00:00
Outdated
Review
Copy Link

use state.availableMigrators?.length > 0

use `state.availableMigrators?.length > 0`
migratorsEnabled: state => state.availableMigrators?.length > 0,
apiBase() {
const {host, protocol} = parseURL(window.API_URL)
return protocol + '//' + host
},
},
mutations: {
[CONFIG](state, config) {
state.version = config.version
@ -63,7 +71,7 @@ export default {
async update(ctx) {
const HTTP = HTTPFactory()
const { data: info } = await HTTP.get('info')
const {data: info} = await HTTP.get('info')
ctx.commit(CONFIG, info)
return info
},

32
src/styles/theme/content.scss
View File

@ -1,20 +1,26 @@
.content h3 {
.icon,
.is-small {
font-size: 1rem;
}
.icon,
.is-small {
font-size: 1rem;
}
}
.table.has-actions {
border-top: 1px solid $grey-100;
border-radius: 4px;
overflow: hidden;
border-top: 1px solid $grey-100;
border-radius: 4px;
overflow: hidden;
td {
vertical-align: middle;
}
td {
vertical-align: middle;
}
td.actions {
text-align: right;
}
td.actions {
text-align: right;
}
}
.content-widescreen {
margin: 0 auto;
max-width: $widescreen;
}

2
src/urls.js Normal file
View File

@ -0,0 +1,2 @@
export const POWERED_BY = 'https://vikunja.io'
export const CALDAV_DOCS = 'https://vikunja.io/docs/caldav/'

1
src/views/user/Login.vue
View File

@ -43,6 +43,7 @@
<label class="label" for="totpPasscode">{{ $t('user.auth.totpTitle') }}</label>
<div class="control">
<input
autocomplete="one-time-code"
class="input"
id="totpPasscode"
:placeholder="$t('user.auth.totpPlaceholder')"

551
src/views/user/Settings.vue
View File

@ -1,469 +1,116 @@
<template>
<div
:class="{ 'is-loading': passwordUpdateService.loading || emailUpdateService.loading || totpService.loading }"
class="loader-container is-max-width-desktop">
<!-- General -->
<card :title="$t('user.settings.general.title')" class="general-settings">
<div class="field">
<label class="label" for="newName">{{ $t('user.settings.general.name') }}</label>
<div class="control">
<input
@keyup.enter="updateSettings"
class="input"
id="newName"
:placeholder="$t('user.settings.general.newName')"
type="text"
v-model="settings.name"/>
</div>
</div>
<div class="field">
<label class="label">
{{ $t('user.settings.general.defaultList') }}
</label>
<list-search v-model="defaultList"/>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.emailRemindersEnabled"/>
{{ $t('user.settings.general.emailReminders') }}
</label>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.overdueTasksRemindersEnabled"/>
{{ $t('user.settings.general.overdueReminders') }}
</label>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.discoverableByName"/>
{{ $t('user.settings.general.discoverableByName') }}
</label>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.discoverableByEmail"/>
{{ $t('user.settings.general.discoverableByEmail') }}
</label>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="playSoundWhenDone"/>
{{ $t('user.settings.general.playSoundWhenDone') }}
</label>
</div>
<div class="field">
<label class="is-flex is-align-items-center">
<span>
{{ $t('user.settings.general.weekStart') }}
</span>
<div class="select ml-2">
<select v-model.number="settings.weekStart">
<option value="0">{{ $t('user.settings.general.weekStartSunday') }}</option>
<option value="1">{{ $t('user.settings.general.weekStartMonday') }}</option>
</select>
</div>
</label>
</div>
<div class="field">
<label class="is-flex is-align-items-center">
<span>
{{ $t('user.settings.general.language') }}
</span>
<div class="select ml-2">
<select v-model="language">
<option :value="lang.code" v-for="lang in availableLanguages" :key="lang.code">{{ lang.title }}</option>
</select>
</div>
</label>
</div>
<div class="field">
<label class="is-flex is-align-items-center">
<span>
{{ $t('user.settings.quickAddMagic.title') }}
</span>
<div class="select ml-2">
<select v-model="quickAddMagicMode">
<option v-for="set in quickAddMagicPrefixes" :key="set" :value="set">{{ $t(`user.settings.quickAddMagic.${set}`) }}</option>
</select>
</div>
</label>
</div>
<x-button
:loading="userSettingsService.loading"
@click="updateSettings()"
class="is-fullwidth mt-4"
>
{{ $t('misc.save') }}
</x-button>
</card>
<!-- Avatar -->
<avatar-settings/>
<!-- Password update -->
<card :title="$t('user.settings.newPasswordTitle')">
<form @submit.prevent="updatePassword()">
<div class="field">
<label class="label" for="newPassword">{{ $t('user.settings.newPassword') }}</label>
<div class="control">
<input
@keyup.enter="updatePassword"
class="input"
id="newPassword"
:placeholder="$t('user.auth.passwordPlaceholder')"
type="password"
v-model="passwordUpdate.newPassword"/>
</div>
</div>
<div class="field">
<label class="label" for="newPasswordConfirm">{{ $t('user.settings.newPasswordConfirm') }}</label>
<div class="control">
<input
@keyup.enter="updatePassword"
class="input"
id="newPasswordConfirm"
:placeholder="$t('user.auth.passwordPlaceholder')"
type="password"
v-model="passwordConfirm"/>
</div>
</div>
<div class="field">
<label class="label" for="currentPassword">{{ $t('user.settings.currentPassword') }}</label>
<div class="control">
<input
@keyup.enter="updatePassword"
class="input"
id="currentPassword"
:placeholder="$t('user.settings.currentPasswordPlaceholder')"
type="password"
v-model="passwordUpdate.oldPassword"/>
</div>
</div>
</form>
<x-button
:loading="passwordUpdateService.loading"
@click="updatePassword()"
class="is-fullwidth mt-4">
{{ $t('misc.save') }}
</x-button>
</card>
<!-- Update E-Mail -->
<card :title="$t('user.settings.updateEmailTitle')">
<form @submit.prevent="updateEmail()">
<div class="field">
<label class="label" for="newEmail">{{ $t('user.settings.updateEmailNew') }}</label>
<div class="control">
<input
@keyup.enter="updateEmail"
class="input"
id="newEmail"
:placeholder="$t('user.auth.emailPlaceholder')"
type="email"
v-model="emailUpdate.newEmail"/>
</div>
</div>
<div class="field">
<label class="label" for="currentPasswordEmail">{{ $t('user.settings.currentPassword') }}</label>
<div class="control">
<input
@keyup.enter="updateEmail"
class="input"
id="currentPasswordEmail"
:placeholder="$t('user.settings.currentPasswordPlaceholder')"
type="password"
v-model="emailUpdate.password"/>
</div>
</div>
</form>
<x-button
:loading="emailUpdateService.loading"
@click="updateEmail()"
class="is-fullwidth mt-4">
{{ $t('misc.save') }}
</x-button>
</card>
<!-- TOTP -->
<card :title="$t('user.settings.totp.title')" v-if="totpEnabled">
<x-button
:loading="totpService.loading"
@click="totpEnroll()"
v-if="!totpEnrolled && totp.secret === ''">
{{ $t('user.settings.totp.enroll') }}
</x-button>
<template v-else-if="totp.secret !== '' && !totp.enabled">
<p>
{{ $t('user.settings.totp.finishSetupPart1') }}
<strong>{{ totp.secret }}</strong><br/>
{{ $t('user.settings.totp.finishSetupPart2') }}
</p>
<p>
{{ $t('user.settings.totp.scanQR') }}<br/>
<img :src="totpQR" alt=""/>
</p>
<div class="field">
<label class="label" for="totpConfirmPasscode">{{ $t('user.settings.totp.passcode') }}</label>
<div class="control">
<input
@keyup.enter="totpConfirm()"
class="input"
id="totpConfirmPasscode"
:placeholder="$t('user.settings.totp.passcodePlaceholder')"
type="text"
v-model="totpConfirmPasscode"/>
</div>
</div>
<x-button @click="totpConfirm()">{{ $t('misc.confirm') }}</x-button>
</template>
<template v-else-if="totp.secret !== '' && totp.enabled">
<p>
{{ $t('user.settings.totp.setupSuccess') }}
</p>
<p v-if="!totpDisableForm">
<x-button @click="totpDisableForm = true" class="is-danger">{{ $t('misc.disable') }}</x-button>
</p>
<div v-if="totpDisableForm">
<div class="field">
<label class="label" for="currentPassword">{{ $t('user.settings.totp.enterPassword') }}</label>
<div class="control">
<input
@keyup.enter="totpDisable"
class="input"
id="currentPassword"
:placeholder="$t('user.settings.currentPasswordPlaceholder')"
type="password"
v-focus
v-model="totpDisablePassword"/>
</div>
</div>
<x-button @click="totpDisable()" class="is-danger">
{{ $t('user.settings.totp.disable') }}
</x-button>
</div>
</template>
</card>
<!-- Data export -->
<data-export/>
<!-- Migration -->
<card :title="$t('migrate.title')" v-if="migratorsEnabled">
<x-button
:to="{name: 'migrate.start'}"
>
{{ $t('migrate.import') }}
</x-button>
</card>
<!-- Account deletion -->
<user-settings-deletion id="deletion"/>
<!-- Caldav -->
<card v-if="caldavEnabled" :title="$t('user.settings.caldav.title')">
<p>
{{ $t('user.settings.caldav.howTo') }}
</p>
<div class="field has-addons no-input-mobile">
<div class="control is-expanded">
<input type="text" v-model="caldavUrl" class="input" readonly/>
</div>
<div class="control">
<x-button
@click="copy(caldavUrl)"
:shadow="false"
v-tooltip="$t('misc.copy')"
icon="paste"
/>
</div>
</div>
<p>
<a href="https://vikunja.io/docs/caldav/" rel="noreferrer noopener nofollow" target="_blank">
{{ $t('user.settings.caldav.more') }}
</a>
</p>
</card>
<div class="content-widescreen">
<div class="user-settings">
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 10:55:31 +00:00
Outdated
Review
Copy Link

Wrap ul in <nav> element

Wrap ul in `<nav>` element
konrad commented 2021-10-25 19:33:05 +00:00
Outdated
Review
Copy Link

Done!

Done!
🎉 1
<nav class="navigation">
<ul>
<li>
<router-link :to="{name: 'user.settings.general'}">
{{ $t('user.settings.general.title') }}
</router-link>
</li>
<li>
<router-link :to="{name: 'user.settings.password-update'}">
{{ $t('user.settings.newPasswordTitle') }}
</router-link>
</li>
<li>
<router-link :to="{name: 'user.settings.email-update'}">
{{ $t('user.settings.updateEmailTitle') }}
</router-link>
</li>
<li>
<router-link :to="{name: 'user.settings.avatar'}">
{{ $t('user.settings.avatar.title') }}
</router-link>
</li>
<li v-if="totpEnabled">
<router-link :to="{name: 'user.settings.totp'}">
{{ $t('user.settings.totp.title') }}
</router-link>
</li>
<li>
<router-link :to="{name: 'user.settings.data-export'}">
{{ $t('user.export.title') }}
</router-link>
</li>
<li v-if="migratorsEnabled">
<router-link :to="{name: 'migrate.start'}">
{{ $t('migrate.title') }}
</router-link>
</li>
<li v-if="caldavEnabled">
<router-link :to="{name: 'user.settings.caldav'}">
{{ $t('user.settings.caldav.title') }}
</router-link>
</li>
<li>
<router-link :to="{name: 'user.settings.deletion'}">
{{ $t('user.deletion.title') }}
</router-link>
</li>
</ul>
</nav>
<section class="view">
<router-view/>
</section>
</div>
</div>
</template>
<script>
import PasswordUpdateModel from '../../models/passwordUpdate'
import PasswordUpdateService from '../../services/passwordUpdateService'
import EmailUpdateService from '../../services/emailUpdate'
import EmailUpdateModel from '../../models/emailUpdate'
import TotpModel from '../../models/totp'
import TotpService from '../../services/totp'
import UserSettingsService from '../../services/userSettings'
import {playSoundWhenDoneKey} from '@/helpers/playPop'
import {availableLanguages, saveLanguage, getCurrentLanguage} from '@/i18n'
import {getQuickAddMagicMode, setQuickAddMagicMode} from '../../helpers/quickAddMagicMode'
import {PrefixMode} from '../../modules/parseTaskText'
import {mapState} from 'vuex'
import AvatarSettings from '../../components/user/avatar-settings.vue'
import copy from 'copy-to-clipboard'
import ListSearch from '@/components/tasks/partials/listSearch.vue'
import UserSettingsDeletion from '../../components/user/settings/deletion'
import DataExport from '../../components/user/settings/data-export'
function getPlaySoundWhenDoneSetting() {
return localStorage.getItem(playSoundWhenDoneKey) === 'true' || localStorage.getItem(playSoundWhenDoneKey) === null
}
export default {
name: 'Settings',
data() {
return {
passwordUpdateService: new PasswordUpdateService(),
passwordUpdate: new PasswordUpdateModel(),
passwordConfirm: '',
emailUpdateService: new EmailUpdateService(),
emailUpdate: new EmailUpdateModel(),
totpService: new TotpService(),
totp: new TotpModel(),
totpQR: '',
totpEnrolled: false,
totpConfirmPasscode: '',
totpDisableForm: false,
totpDisablePassword: '',
playSoundWhenDone: getPlaySoundWhenDoneSetting(),
language: getCurrentLanguage(),
quickAddMagicMode: getQuickAddMagicMode(),
quickAddMagicPrefixes: PrefixMode,
settings: { ...this.$store.state.auth.settings },
userSettingsService: new UserSettingsService(),
}
},
components: {
UserSettingsDeletion,
ListSearch,
AvatarSettings,
DataExport,
},
created() {
this.totpStatus()
},
mounted() {
this.setTitle(this.$t('user.settings.title'))
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 10:58:36 +00:00
Outdated
Review
Copy Link

How about making migratorsEnabled a getter in the store?

How about making `migratorsEnabled` a getter in the store?
dpschen commented 2021-10-23 10:59:46 +00:00
Outdated
Review
Copy Link

The rest can be simplified with:

computed: {
	...mapState('config', ['totpEnabled', 'caldavEnabled'],
	[...]
}
The rest can be simplified with: ```js computed: { ...mapState('config', ['totpEnabled', 'caldavEnabled'], [...] } ```
konrad commented 2021-10-25 19:37:08 +00:00
Outdated
Review
Copy Link

Done (both of it).

Done (both of it).
🎉 1
this.anchorHashCheck()
},
computed: {
defaultList() {
return this.$store.getters['lists/getListById'](this.settings.defaultListId)
},
caldavUrl() {
let apiBase = window.API_URL.replace('/api/v1', '')
if (apiBase === '') { // Frontend and api on the same host which means we need to prefix the frontend url
apiBase = this.$store.state.config.frontendUrl
}
if (apiBase.endsWith('/')) {
apiBase = apiBase.substr(0, apiBase.length - 1)
}
return `${apiBase}/dav/principals/${this.userInfo.username}/`
},
availableLanguages() {
return Object.entries(availableLanguages)
.map(l => ({code: l[0], title: l[1]}))
.sort((a, b) => a.title > b.title)
},
...mapState({
totpEnabled: state => state.config.totpEnabled,
migratorsEnabled: state => state.config.availableMigrators !== null && state.config.availableMigrators.length > 0,
caldavEnabled: state => state.config.caldavEnabled,
userInfo: state => state.auth.info,
}),
},
methods: {
copy,
async updatePassword() {
if (this.passwordConfirm !== this.passwordUpdate.newPassword) {
this.$message.error({message: this.$t('user.settings.passwordsDontMatch')})
return
}
await this.passwordUpdateService.update(this.passwordUpdate)
this.$message.success({message: this.$t('user.settings.passwordUpdateSuccess')})
},
async updateEmail() {
await this.emailUpdateService.update(this.emailUpdate)
this.$message.success({message: this.$t('user.settings.updateEmailSuccess')})
},
async totpStatus() {
if (!this.totpEnabled) {
return
}
try {
this.totp = await this.totpService.get()
this.totpSetQrCode()
} catch(e) {
// Error code 1016 means totp is not enabled, we don't need an error in that case.
if (e.response && e.response.data && e.response.data.code && e.response.data.code === 1016) {
this.totpEnrolled = false
return
}
throw e
}
},
async totpSetQrCode() {
const qr = await this.totpService.qrcode()
const urlCreator = window.URL || window.webkitURL
this.totpQR = urlCreator.createObjectURL(qr)
},
async totpEnroll() {
this.totp = await this.totpService.enroll()
this.totpEnrolled = true
this.totpSetQrCode()
},
async totpConfirm() {
await this.totpService.enable({passcode: this.totpConfirmPasscode})
this.totp.enabled = true
this.$message.success({message: this.$t('user.settings.totp.confirmSuccess')})
},
async totpDisable() {
await this.totpService.disable({password: this.totpDisablePassword})
this.totpEnrolled = false
this.totp = new TotpModel()
this.$message.success({message: this.$t('user.settings.totp.disableSuccess')})
},
async updateSettings() {
localStorage.setItem(playSoundWhenDoneKey, this.playSoundWhenDone)
saveLanguage(this.language)
setQuickAddMagicMode(this.quickAddMagicMode)
this.settings.defaultListId = this.defaultList ? this.defaultList.id : 0
await this.userSettingsService.update(this.settings)
this.$store.commit('auth/setUserSettings', {
...this.settings,
})
this.$message.success({message: this.$t('user.settings.general.savedSuccess')})
},
anchorHashCheck() {
if (window.location.hash === this.$route.hash) {
const el = document.getElementById(this.$route.hash.slice(1))
if (el) {
window.scrollTo(0, el.offsetTop)
}
}
...mapState('config', ['totpEnabled', 'caldavEnabled']),
migratorsEnabled() {
return this.$store.getters['config/migratorsEnabled']
},
},
}
</script>
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:03:20 +00:00
Outdated
Review
Copy Link

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.

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.
konrad commented 2021-10-25 19:38:09 +00:00
Outdated
Review
Copy Link

You mean wrapping the <router-view> this is mounted in with a margin: 0 auto?

You mean wrapping the `<router-view>` this is mounted in with a `margin: 0 auto`?
dpschen commented 2021-10-25 19:47:25 +00:00
Outdated
Review
Copy Link

No. I guess that also doesn't make sense.

My idea was

  • either to create a wrapper component for similar usecases where the view is centered
  • or to create a wrapper div inside this component so that the centered content styling is encapsulated in this component.

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: fixed to 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.

No. I guess that also doesn't make sense. My idea was - either to create a wrapper component for similar usecases where the view is centered - or to create a wrapper div inside this component so that the centered content styling is encapsulated in this component. 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: fixed` to 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.
konrad commented 2021-10-25 19:58:11 +00:00
Outdated
Review
Copy Link

I see, thanks for clarifying. Sounds very reasonable.

I've added a wrapper div in the component, hope that clears it up.

I see, thanks for clarifying. Sounds very reasonable. I've added a wrapper div in the component, hope that clears it up.
dpschen commented 2021-10-25 19:59:38 +00:00
Outdated
Review
Copy Link

Sounds good!

I think abstracting this to another component just makes sense when we have a usecase somewhere else =)

Sounds good! I think abstracting this to another component just makes sense when we have a usecase somewhere else =)
<style lang="scss" scoped>
.user-settings {
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:14:25 +00:00
Outdated
Review
Copy Link

ul, and li a don't have to be nested.

Better would be to add classes to the elements because they make maintaining the CSS simpler by:

  • making overwrites simpler. This is because tags have a higher specifity.
  • making it simpler to change the tag of the affected elements later.
`ul`, and `li a` don't have to be nested. Better would be to add classes to the elements because they make maintaining the CSS simpler by: - making overwrites simpler. This is because tags have a higher specifity. - making it simpler to change the tag of the affected elements later.
konrad commented 2021-10-25 19:33:21 +00:00
Outdated
Review
Copy Link

Done.

Done.
🎉 1
display: flex;
.navigation {
width: 25%;
padding-right: 1rem;
a {
display: block;
padding: .5rem;
color: $dark;
width: 100%;
border-left: 3px solid transparent;
&:hover, &.router-link-active {
background: $white;
border-color: $primary;
}
}
}
.view {
width: 75%;
}
@media screen and (max-width: $tablet) {
flex-direction: column;
.navigation, .view {
width: 100%;
padding-left: 0;
}
.view {
padding-top: 1rem;
}
}
}
</style>

9
src/components/user/avatar-settings.vue → src/views/user/settings/Avatar.vue
View File

@ -66,11 +66,11 @@
import {Cropper} from 'vue-advanced-cropper'
import 'vue-advanced-cropper/dist/style.css'
import AvatarService from '../../services/avatar'
import AvatarModel from '../../models/avatar'
import AvatarService from '@/services/avatar'
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:15:58 +00:00
Outdated
Review
Copy Link

Use @ alias so that we don't have to rewrite import paths every time we move a component to a new folder

Use `@` alias so that we don't have to rewrite import paths every time we move a component to a new folder
konrad commented 2021-10-25 19:40:27 +00:00
Outdated
Review
Copy Link

Yeah that's something I need to teach webstorm so it doesn't mess it up all the time.

Adjusted in all new components.

Yeah that's something I need to teach webstorm so it doesn't mess it up all the time. Adjusted in all new components.
🎉 1
import AvatarModel from '@/models/avatar'
export default {
name: 'avatar-settings',
name: 'user-settings-avatar',
data() {
return {
avatarProvider: '',
@ -86,6 +86,9 @@ export default {
components: {
Cropper,
},
mounted() {
this.setTitle(`${this.$t('user.settings.avatar.title')} - ${this.$t('user.settings.title')}`)
},
methods: {
async avatarStatus() {
const { avatarProvider } = await this.avatarService.get({})

55
src/views/user/settings/Caldav.vue Normal file
View File

@ -0,0 +1,55 @@
<template>
<card v-if="caldavEnabled" :title="$t('user.settings.caldav.title')">
dpschen marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:17:58 +00:00
Outdated
Review
Copy Link

This would show a empty view if caldav gets disabled out of whatever reason.

How about using the beforeRouteEnter hook, redirect to the general settings and throwing an error?

This would show a empty view if caldav gets disabled out of whatever reason. How about using the `beforeRouteEnter` hook, redirect to the general settings and throwing an error?
konrad commented 2021-10-25 19:45:08 +00:00
Outdated
Review
Copy Link

How would I access the config in state? this doesn't seem to be available in the before hook.

How would I access the config in state? `this` doesn't seem to [be available](https://router.vuejs.org/guide/advanced/navigation-guards.html#in-component-guards) in the before hook.
dpschen commented 2021-10-25 19:50:39 +00:00
Outdated
Review
Copy Link

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.

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.
konrad commented 2021-10-25 20:02:27 +00:00
Outdated
Review
Copy Link

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.

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.
dpschen commented 2021-10-26 20:20:47 +00:00
Outdated
Review
Copy Link

Solving this with new issue: #906

Solving this with new issue: https://kolaente.dev/vikunja/frontend/issues/906
<p>
{{ $t('user.settings.caldav.howTo') }}
</p>
<div class="field has-addons no-input-mobile">
<div class="control is-expanded">
<input type="text" v-model="caldavUrl" class="input" readonly/>
</div>
<div class="control">
<x-button
@click="copy(caldavUrl)"
:shadow="false"
v-tooltip="$t('misc.copy')"
icon="paste"
/>
</div>
</div>
<p>
<a href="https://vikunja.io/docs/caldav/" rel="noreferrer noopener nofollow" target="_blank">
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:21:05 +00:00
Outdated
Review
Copy Link

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.

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.
konrad commented 2021-10-25 19:49:37 +00:00
Outdated
Review
Copy Link

Makes sense. Done.

Makes sense. Done.
🎉 1
{{ $t('user.settings.caldav.more') }}
</a>
</p>
</card>
</template>
<script>
import copy from 'copy-to-clipboard'
import {mapState} from 'vuex'
import {CALDAV_DOCS} from '@/urls'
export default {
name: 'user-settings-caldav',
data() {
return {
caldavDocsUrl: CALDAV_DOCS,
}
},
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:29:36 +00:00
Outdated
Review
Copy Link

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.vue

How 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.

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.vue` How 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.
👍 1
konrad commented 2021-10-25 20:29:54 +00:00
Outdated
Review
Copy Link

That really is a hack from ancient times...

Moved it to a getter.

That really is a hack from ancient times... Moved it to a getter.
mounted() {
this.setTitle(`${this.$t('user.settings.caldav.title')} - ${this.$t('user.settings.title')}`)
},
computed: {
caldavUrl() {
return `${this.$store.getters['config/apiBase']}/dav/principals/${this.userInfo.username}/`
},
...mapState('config', ['caldavEnabled']),
...mapState({
userInfo: state => state.auth.info,
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:30:15 +00:00
Outdated
Review
Copy Link

See comment above regariding usage of mapState

See comment above regariding usage of `mapState`
konrad commented 2021-10-25 19:51:14 +00:00
Outdated
Review
Copy Link

Even though the two variables from state I'm using here come from different components?

Even though the two variables from state I'm using here come from different components?
dpschen commented 2021-10-25 19:57:52 +00:00
Outdated
Review
Copy Link

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 (caldavEnabled once as prop in the store and once as the local representation — we want it to be the same either way).

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 (`caldavEnabled` once as prop in the store and once as the local representation — we want it to be the same either way).
konrad commented 2021-10-26 19:13:12 +00:00
Outdated
Review
Copy Link

I've change the config one but kept the other one. I wanted to keep having auth.info as userinfo instead of just info which it would have been if I'd just use mapState('auth', ['info']). Hope that makes sense.

I've change the config one but kept the other one. I wanted to keep having `auth.info` as `userinfo` instead of just `info` which it would have been if I'd just use `mapState('auth', ['info'])`. Hope that makes sense.
👍 1
}),
},
methods: {
copy,
},
}
</script>

7
src/components/user/settings/data-export.vue → src/views/user/settings/DataExport.vue
View File

@ -37,10 +37,10 @@
</template>
<script>
import DataExportService from '../../../services/dataExport'
import DataExportService from '@/services/dataExport'
export default {
name: 'data-export',
name: 'user-settings-data-export',
data() {
return {
dataExportService: new DataExportService(),
@ -48,6 +48,9 @@ export default {
errPasswordRequired: false,
}
},
mounted() {
this.setTitle(`${this.$t('user.export.title')} - ${this.$t('user.settings.title')}`)
},
methods: {
async requestDataExport() {
if (this.password === '') {

7
src/components/user/settings/deletion.vue → src/views/user/settings/Deletion.vue
View File

@ -83,9 +83,9 @@
</template>
<script>
import AccountDeleteService from '../../../services/accountDelete'
import AccountDeleteService from '@/services/accountDelete'
import {mapState} from 'vuex'
import {parseDateOrNull} from '../../../helpers/parseDateOrNull'
import {parseDateOrNull} from '@/helpers/parseDateOrNull'
export default {
name: 'user-settings-deletion',
@ -100,6 +100,9 @@ export default {
userDeletionEnabled: state => state.config.userDeletionEnabled,
deletionScheduledAt: state => parseDateOrNull(state.auth.info.deletionScheduledAt),
}),
mounted() {
this.setTitle(`${this.$t('user.deletion.title')} - ${this.$t('user.settings.title')}`)
},
methods: {
async deleteAccount() {
if (this.password === '') {

61
src/views/user/settings/EmailUpdate.vue Normal file
View File

@ -0,0 +1,61 @@
<template>
<card :title="$t('user.settings.updateEmailTitle')">
<form @submit.prevent="updateEmail">
<div class="field">
<label class="label" for="newEmail">{{ $t('user.settings.updateEmailNew') }}</label>
<div class="control">
<input
@keyup.enter="updateEmail"
class="input"
id="newEmail"
:placeholder="$t('user.auth.emailPlaceholder')"
type="email"
v-model="emailUpdate.newEmail"/>
</div>
</div>
<div class="field">
<label class="label" for="currentPasswordEmail">{{ $t('user.settings.currentPassword') }}</label>
<div class="control">
<input
@keyup.enter="updateEmail"
class="input"
id="currentPasswordEmail"
:placeholder="$t('user.settings.currentPasswordPlaceholder')"
type="password"
v-model="emailUpdate.password"/>
</div>
</div>
</form>
<x-button
:loading="emailUpdateService.loading"
@click="updateEmail"
class="is-fullwidth mt-4">
{{ $t('misc.save') }}
</x-button>
</card>
</template>
<script>
import EmailUpdateService from '@/services/emailUpdate'
import EmailUpdateModel from '@/models/emailUpdate'
export default {
name: 'user-settings-update-email',
data() {
return {
emailUpdateService: new EmailUpdateService(),
emailUpdate: new EmailUpdateModel(),
}
},
mounted() {
this.setTitle(`${this.$t('user.settings.updateEmailTitle')} - ${this.$t('user.settings.title')}`)
},
methods: {
async updateEmail() {
dpschen commented 2021-10-23 11:33:53 +00:00
Outdated
Review
Copy Link

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 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.
konrad commented 2021-10-25 20:22:45 +00:00
Outdated
Review
Copy Link

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?

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?
dpschen commented 2021-10-25 20:29:17 +00:00
Outdated
Review
Copy Link

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.

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.
👍 1
konrad commented 2021-10-26 19:16:25 +00:00
Outdated
Review
Copy Link

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?

If it's just the undo action, you could probably get away with just using an arrow function. (that actually works already, see [here](https://kolaente.dev/vikunja/frontend/src/branch/main/src/views/tasks/TaskDetailView.vue#L644)). 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?
await this.emailUpdateService.update(this.emailUpdate)
this.$message.success({message: this.$t('user.settings.updateEmailSuccess')})
},
},
}
</script>

169
src/views/user/settings/General.vue Normal file
View File

@ -0,0 +1,169 @@
<template>
<card :title="$t('user.settings.general.title')" class="general-settings" :loading="userSettingsService.loading">
<div class="field">
<label class="label" :for="`newName${id}`">{{ $t('user.settings.general.name') }}</label>
<div class="control">
<input
@keyup.enter="updateSettings"
class="input"
:id="`newName${id}`"
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:35:46 +00:00
Outdated
Review
Copy Link

The id should include a random id to prevent future errors of duplicate ids.

The id should include a random id to prevent future errors of duplicate ids.
konrad commented 2021-10-25 20:05:51 +00:00
Outdated
Review
Copy Link

Done.

Done.
🎉 1
:placeholder="$t('user.settings.general.newName')"
type="text"
v-model="settings.name"/>
</div>
</div>
<div class="field">
<label class="label">
{{ $t('user.settings.general.defaultList') }}
</label>
<list-search v-model="defaultList"/>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.emailRemindersEnabled"/>
{{ $t('user.settings.general.emailReminders') }}
</label>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.overdueTasksRemindersEnabled"/>
{{ $t('user.settings.general.overdueReminders') }}
</label>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.discoverableByName"/>
{{ $t('user.settings.general.discoverableByName') }}
</label>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.discoverableByEmail"/>
{{ $t('user.settings.general.discoverableByEmail') }}
</label>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="playSoundWhenDone"/>
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:57:11 +00:00
Outdated
Review
Copy Link

When this gets changed to true we should play the actual sound.

When this gets changed to `true` we should play the actual sound.
{{ $t('user.settings.general.playSoundWhenDone') }}
</label>
</div>
<div class="field">
<label class="is-flex is-align-items-center">
<span>
{{ $t('user.settings.general.weekStart') }}
</span>
<div class="select ml-2">
<select v-model.number="settings.weekStart">
<option value="0">{{ $t('user.settings.general.weekStartSunday') }}</option>
<option value="1">{{ $t('user.settings.general.weekStartMonday') }}</option>
</select>
</div>
</label>
</div>
<div class="field">
<label class="is-flex is-align-items-center">
<span>
{{ $t('user.settings.general.language') }}
</span>
<div class="select ml-2">
<select v-model="language">
<option :value="lang.code" v-for="lang in availableLanguages" :key="lang.code">{{
lang.title
}}
</option>
</select>
</div>
</label>
</div>
<div class="field">
<label class="is-flex is-align-items-center">
<span>
{{ $t('user.settings.quickAddMagic.title') }}
</span>
<div class="select ml-2">
<select v-model="quickAddMagicMode">
<option v-for="set in quickAddMagicPrefixes" :key="set" :value="set">
{{ $t(`user.settings.quickAddMagic.${set}`) }}
</option>
</select>
</div>
</label>
</div>
<x-button
:loading="userSettingsService.loading"
@click="updateSettings()"
class="is-fullwidth mt-4"
>
{{ $t('misc.save') }}
</x-button>
</card>
</template>
<script>
import {playSoundWhenDoneKey} from '@/helpers/playPop'
import {availableLanguages, saveLanguage, getCurrentLanguage} from '@/i18n'
import {getQuickAddMagicMode, setQuickAddMagicMode} from '@/helpers/quickAddMagicMode'
import UserSettingsService from '@/services/userSettings'
import {PrefixMode} from '@/modules/parseTaskText'
import ListSearch from '@/components/tasks/partials/listSearch'
import {createRandomID} from '@/helpers/randomId'
import {playPop} from '@/helpers/playPop'
function getPlaySoundWhenDoneSetting() {
return localStorage.getItem(playSoundWhenDoneKey) === 'true' || localStorage.getItem(playSoundWhenDoneKey) === null
}
export default {
name: 'user-settings-general',
data() {
return {
playSoundWhenDone: getPlaySoundWhenDoneSetting(),
language: getCurrentLanguage(),
quickAddMagicMode: getQuickAddMagicMode(),
quickAddMagicPrefixes: PrefixMode,
userSettingsService: new UserSettingsService(),
settings: {...this.$store.state.auth.settings},
id: createRandomID(),
}
},
components: {
ListSearch,
},
computed: {
availableLanguages() {
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:38:12 +00:00
Outdated
Review
Copy Link

We should use localCompare for sorting by abc

We should use [localCompare](https://stackoverflow.com/a/45544166/15522256) for sorting by abc
return Object.entries(availableLanguages)
.map(l => ({code: l[0], title: l[1]}))
.sort((a, b) => a.title.localeCompare(b.title))
},
defaultList() {
return this.$store.getters['lists/getListById'](this.settings.defaultListId)
},
},
mounted() {
this.setTitle(`${this.$t('user.settings.general.title')} - ${this.$t('user.settings.title')}`)
},
watch: {
playSoundWhenDone(play) {
if (play) {
playPop()
}
},
},
methods: {
async updateSettings() {
localStorage.setItem(playSoundWhenDoneKey, this.playSoundWhenDone)
saveLanguage(this.language)
setQuickAddMagicMode(this.quickAddMagicMode)
this.settings.defaultListId = this.defaultList ? this.defaultList.id : 0
await this.userSettingsService.update(this.settings)
this.$store.commit('auth/setUserSettings', {
...this.settings,
})
this.$message.success({message: this.$t('user.settings.general.savedSuccess')})
},
},
}
</script>

82
src/views/user/settings/PasswordUpdate.vue Normal file
View File

@ -0,0 +1,82 @@
<template>
<card :title="$t('user.settings.newPasswordTitle')" :loading="passwordUpdateService.loading">
<form @submit.prevent="updatePassword">
<div class="field">
<label class="label" for="newPassword">{{ $t('user.settings.newPassword') }}</label>
<div class="control">
<input
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:44:03 +00:00
Outdated
Review
Copy Link

We should add the correct autocomplete attributes so that the changed login data is easier available in password managers.

We should add [the correct autocomplete attributes](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete#values) so that the changed login data is easier available in password managers.
autocomplete="new-password"
@keyup.enter="updatePassword"
class="input"
id="newPassword"
:placeholder="$t('user.auth.passwordPlaceholder')"
type="password"
v-model="passwordUpdate.newPassword"/>
</div>
</div>
<div class="field">
<label class="label" for="newPasswordConfirm">{{ $t('user.settings.newPasswordConfirm') }}</label>
<div class="control">
<input
autocomplete="new-password"
@keyup.enter="updatePassword"
class="input"
id="newPasswordConfirm"
:placeholder="$t('user.auth.passwordPlaceholder')"
type="password"
v-model="passwordConfirm"/>
</div>
</div>
<div class="field">
<label class="label" for="currentPassword">{{ $t('user.settings.currentPassword') }}</label>
<div class="control">
<input
autocomplete="current-password"
@keyup.enter="updatePassword"
class="input"
id="currentPassword"
:placeholder="$t('user.settings.currentPasswordPlaceholder')"
type="password"
v-model="passwordUpdate.oldPassword"/>
</div>
</div>
</form>
<x-button
:loading="passwordUpdateService.loading"
@click="updatePassword"
class="is-fullwidth mt-4">
{{ $t('misc.save') }}
</x-button>
</card>
</template>
<script>
import PasswordUpdateService from '@/services/passwordUpdateService'
import PasswordUpdateModel from '@/models/passwordUpdate'
export default {
name: 'user-settings-password-update',
data() {
return {
passwordUpdateService: new PasswordUpdateService(),
passwordUpdate: new PasswordUpdateModel(),
passwordConfirm: '',
}
},
mounted() {
this.setTitle(`${this.$t('user.settings.newPasswordTitle')} - ${this.$t('user.settings.title')}`)
},
methods: {
async updatePassword() {
if (this.passwordConfirm !== this.passwordUpdate.newPassword) {
this.$message.error({message: this.$t('user.settings.passwordsDontMatch')})
return
}
await this.passwordUpdateService.update(this.passwordUpdate)
this.$message.success({message: this.$t('user.settings.passwordUpdateSuccess')})
},
},
}
</script>

133
src/views/user/settings/TOTP.vue Normal file
View File

@ -0,0 +1,133 @@
<template>
<card :title="$t('user.settings.totp.title')" v-if="totpEnabled">
dpschen marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:45:07 +00:00
Outdated
Review
Copy Link

Same here with the beforeRouteEnter.

Same here with the `beforeRouteEnter`.
dpschen commented 2021-10-26 20:21:24 +00:00
Outdated
Review
Copy Link

See new issue: #906

See new issue: https://kolaente.dev/vikunja/frontend/issues/906
<x-button
:loading="totpService.loading"
@click="totpEnroll()"
v-if="!totpEnrolled && totp.secret === ''">
{{ $t('user.settings.totp.enroll') }}
</x-button>
<template v-else-if="totp.secret !== '' && !totp.enabled">
<p>
{{ $t('user.settings.totp.finishSetupPart1') }}
<strong>{{ totp.secret }}</strong><br/>
{{ $t('user.settings.totp.finishSetupPart2') }}
</p>
<p>
{{ $t('user.settings.totp.scanQR') }}<br/>
<img :src="totpQR" alt=""/>
</p>
<div class="field">
<label class="label" for="totpConfirmPasscode">{{ $t('user.settings.totp.passcode') }}</label>
<div class="control">
<input
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:47:39 +00:00
Outdated
Review
Copy Link

Add autocomplete="one-time-code"

Add `autocomplete="one-time-code"`
autocomplete="one-time-code"
@keyup.enter="totpConfirm"
class="input"
id="totpConfirmPasscode"
:placeholder="$t('user.settings.totp.passcodePlaceholder')"
type="text"
v-model="totpConfirmPasscode"/>
</div>
</div>
<x-button @click="totpConfirm">{{ $t('misc.confirm') }}</x-button>
</template>
<template v-else-if="totp.secret !== '' && totp.enabled">
<p>
{{ $t('user.settings.totp.setupSuccess') }}
</p>
<p v-if="!totpDisableForm">
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 11:54:34 +00:00
Outdated
Review
Copy Link

This should be a toggle button so that a curious user can go back in the previous ui state.

This should be a toggle button so that a curious user can go back in the previous ui state.
konrad commented 2021-10-25 20:13:18 +00:00
Outdated
Review
Copy Link

But the button is not visible when totpDisableForm is true. It requires a different action to disable it.

But the button is not visible when `totpDisableForm` is true. It requires a different action to disable it.
dpschen commented 2021-10-25 20:46:03 +00:00
Outdated
Review
Copy Link

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.

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.
konrad commented 2021-10-26 19:25:29 +00:00
Outdated
Review
Copy Link

Good point. I've added a "cancel" button next to the "disable" button which reverses the UI state. I think that should be enough.

Good point. I've added a "cancel" button next to the "disable" button which reverses the UI state. I think that should be enough.
👍 1
<x-button @click="totpDisableForm = true" class="is-danger">{{ $t('misc.disable') }}</x-button>
</p>
<div v-if="totpDisableForm">
<div class="field">
<label class="label" for="currentPassword">{{ $t('user.settings.totp.enterPassword') }}</label>
<div class="control">
<input
@keyup.enter="totpDisable"
class="input"
id="currentPassword"
:placeholder="$t('user.settings.currentPasswordPlaceholder')"
type="password"
v-focus
v-model="totpDisablePassword"/>
</div>
</div>
<x-button @click="totpDisable" class="is-danger">
{{ $t('user.settings.totp.disable') }}
</x-button>
<x-button @click="totpDisableForm = false" type="tertary" class="ml-2">
{{ $t('misc.cancel') }}
</x-button>
</div>
</template>
</card>
</template>
<script>
import TotpService from '@/services/totp'
import TotpModel from '@/models/totp'
import {mapState} from 'vuex'
export default {
name: 'user-settings-totp',
data() {
return {
totpService: new TotpService(),
totp: new TotpModel(),
totpQR: '',
totpEnrolled: false,
totpConfirmPasscode: '',
totpDisableForm: false,
totpDisablePassword: '',
}
},
created() {
this.totpStatus()
},
computed: mapState({
totpEnabled: state => state.config.totpEnabled,
}),
mounted() {
this.setTitle(`${this.$t('user.settings.totp.title')} - ${this.$t('user.settings.title')}`)
},
methods: {
async totpStatus() {
if (!this.totpEnabled) {
return
}
try {
this.totp = await this.totpService.get()
this.totpSetQrCode()
} catch(e) {
// Error code 1016 means totp is not enabled, we don't need an error in that case.
if (e.response && e.response.data && e.response.data.code && e.response.data.code === 1016) {
this.totpEnrolled = false
return
}
throw e
}
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-23 12:00:54 +00:00
Outdated
Review
Copy Link

Remove window.webkitURL. In other places in the codebase window.URL.createObjectURL is called directly.

Remove `window.webkitURL`. In other places in the codebase `window.URL.createObjectURL` is called directly.
},
async totpSetQrCode() {
const qr = await this.totpService.qrcode()
this.totpQR = window.URL.createObjectURL(qr)
},
async totpEnroll() {
this.totp = await this.totpService.enroll()
this.totpEnrolled = true
this.totpSetQrCode()
},
async totpConfirm() {
await this.totpService.enable({passcode: this.totpConfirmPasscode})
this.totp.enabled = true
this.$message.success({message: this.$t('user.settings.totp.confirmSuccess')})
},
async totpDisable() {
await this.totpService.disable({password: this.totpDisablePassword})
this.totpEnrolled = false
this.totp = new TotpModel()
this.$message.success({message: this.$t('user.settings.totp.disableSuccess')})
},
},
}
</script>