feat: improve login and register ux #1104

Merged
dpschen merged 23 commits from feature/login-improvements into main 2022-02-05 17:22:43 +00:00
13 changed files with 255 additions and 119 deletions

View File

@ -25,7 +25,6 @@ context('Registration', () => {
cy.get('#username').type(fixture.username)
cy.get('#email').type(fixture.email)
cy.get('#password').type(fixture.password)
cy.get('#passwordValidation').type(fixture.password)
cy.get('#register-submit').click()
cy.url().should('include', '/')
cy.clock(1625656161057) // 13:00
@ -43,7 +42,6 @@ context('Registration', () => {
cy.get('#username').type(fixture.username)
cy.get('#email').type(fixture.email)
cy.get('#password').type(fixture.password)
cy.get('#passwordValidation').type(fixture.password)
cy.get('#register-submit').click()
cy.get('div.message.danger').contains('A user with this username already exists.')
})

View File

@ -0,0 +1,85 @@
<template>
<div class="password-field">
<input
class="input"
id="password"
name="password"
:placeholder="$t('user.auth.passwordPlaceholder')"
required
:type="passwordFieldType"
autocomplete="current-password"
@keyup.enter="e => $emit('submit', e)"
:tabindex="props.tabindex"
@focusout="validate"
@input="handleInput"
/>
<a
@click="togglePasswordFieldType"
class="password-field-type-toggle"
aria-label="passwordFieldType === 'password' ? $t('user.auth.showPassword') : $t('user.auth.hidePassword')"
v-tooltip="passwordFieldType === 'password' ? $t('user.auth.showPassword') : $t('user.auth.hidePassword')">
<icon :icon="passwordFieldType === 'password' ? 'eye' : 'eye-slash'"/>
</a>
</div>
<p class="help is-danger" v-if="!isValid">
{{ $t('user.auth.passwordRequired') }}
</p>
</template>
<script lang="ts" setup>
import {ref, watch} from 'vue'
import {useDebounceFn} from '@vueuse/core'
const props = defineProps({
tabindex: String,
modelValue: String,
// This prop is a workaround to trigger validation from the outside when the user never had focus in the input.
validateInitially: Boolean,
Review

I'm not quite sure if this is the best way to do it. Feels a lot like a hack.

I'm not quite sure if this is the best way to do it. Feels a lot like a hack.
})
const emit = defineEmits(['submit', 'update:modelValue'])
const passwordFieldType = ref<String>('password')
const password = ref<String>('')
const isValid = ref<Boolean>(!props.validateInitially)
watch(
() => props.validateInitially,
(doValidate: Boolean) => {
if (doValidate) {
validate()
}
},
)
function validate() {
useDebounceFn(() => {
isValid.value = password.value !== ''
}, 100)()
}
function togglePasswordFieldType() {
passwordFieldType.value = passwordFieldType.value === 'password'
? 'text'
: 'password'
}
function handleInput(e) {
password.value = e.target.value
emit('update:modelValue', e.target.value)
}
</script>
<style scoped>
.password-field {
position: relative;
}
.password-field-type-toggle {
position: absolute;
color: var(--grey-400);
top: 50%;
right: 1rem;
transform: translateY(-50%);
}
</style>

View File

@ -1,18 +1,35 @@
<template>
<div class="message-wrapper">
<div class="message" :class="variant">
<div class="message" :class="[variant, textAlignClass]">
<slot/>
</div>
</div>
</template>
<script lang="ts" setup>
defineProps({
import {computed, PropType} from 'vue'
const TEXT_ALIGN_MAP = Object.freeze({
left: '',
center: 'has-text-centered',
right: 'has-text-right',
})
type textAlignVariants = keyof typeof TEXT_ALIGN_MAP
const props = defineProps({
variant: {
type: String,
default: 'info',
},
textAlign: {
type: String as PropType<textAlignVariants>,
default: 'left',
},
})
const textAlignClass = computed(() => TEXT_ALIGN_MAP[props.textAlign])
</script>
<style lang="scss" scoped>

View File

@ -14,6 +14,9 @@
<div>
<h2 class="title" v-if="title">{{ title }}</h2>
<api-config/>
<Message v-if="motd !== ''" class="is-hidden-tablet mb-4">
{{ motd }}
</Message>
<slot/>
</div>
<legal/>
@ -38,8 +41,8 @@ const store = useStore()
const {t} = useI18n()
const motd = computed(() => store.state.config.motd)
// @ts-ignore
const title = computed(() => t(route.meta.title ?? ''))
const title = computed(() => t(route.meta?.title as string || ''))
useTitle(() => title.value)
</script>

6
src/helpers/isEmail.ts Normal file
View File

@ -0,0 +1,6 @@
export function isEmail(email: string): Boolean {
const format = /^.+@.+$/
const match = email.match(format)
return match === null ? false : match.length > 0
}

View File

@ -31,10 +31,9 @@
"username": "Username",
"usernameEmail": "Username Or Email Address",
"usernamePlaceholder": "e.g. frederick",
"email": "E-mail address",
"email": "Email address",
"emailPlaceholder": "e.g. frederic{'@'}vikunja.io",
"password": "Password",
"passwordRepeat": "Retype your password",
"passwordPlaceholder": "e.g. •••••••••••",
"forgotPassword": "Forgot your password?",
"resetPassword": "Reset your password",
@ -45,12 +44,19 @@
"totpTitle": "Two Factor Authentication Code",
"totpPlaceholder": "e.g. 123456",
"login": "Login",
"register": "Register",
"createAccount": "Create account",
konrad marked this conversation as resolved Outdated

This is much better! =)

This is much better! =)
"loginWith": "Log in with {provider}",
"authenticating": "Authenticating…",
"openIdStateError": "State does not match, refusing to continue!",
"openIdGeneralError": "An error occured while authenticating against the third party.",
"logout": "Logout"
"logout": "Logout",
"emailInvalid": "Please enter a valid email address.",
konrad marked this conversation as resolved Outdated

Email is written inconsistent in English: sometimes it's "E-mail" sometimes "Email".
The more common one seems to be "Email" (more google results and wiki article's main spelling).

Email is written inconsistent in English: sometimes it's "E-mail" sometimes "Email". The more common one seems to be "Email" (more google results and wiki article's main spelling).

Changed it so that it's consistent everywhere.

Changed it so that it's consistent everywhere.
"usernameRequired": "Please provide a username.",
"passwordRequired": "Please provide a password.",
"showPassword": "Show the password",
"hidePassword": "Hide the password",
"noAccountYet": "Don't have an account yet?",
"alreadyHaveAnAccount": "Already have an account?"
},
"settings": {
"title": "Settings",
@ -61,7 +67,7 @@
"currentPasswordPlaceholder": "Your current password",
"passwordsDontMatch": "The new password and its confirmation don't match.",
"passwordUpdateSuccess": "The password was successfully updated.",
"updateEmailTitle": "Update Your E-Mail Address",
"updateEmailTitle": "Update Your Email Address",
"updateEmailNew": "New Email Address",
"updateEmailSuccess": "Your email address was successfully updated. We've sent you a link to confirm it.",
"general": {

View File

@ -16,6 +16,8 @@ import {
faCocktail,
faCoffee,
faCog,
faEye,
faEyeSlash,
faEllipsisH,
faEllipsisV,
faExclamation,
@ -87,6 +89,8 @@ library.add(faCocktail)
library.add(faCoffee)
library.add(faCog)
library.add(faComments)
library.add(faEye)
library.add(faEyeSlash)
library.add(faEllipsisH)
library.add(faEllipsisV)
library.add(faExclamation)

View File

@ -133,7 +133,7 @@ const router = createRouter({
name: 'user.register',
component: RegisterComponent,
meta: {
title: 'user.auth.register',
title: 'user.auth.createAccount',
},
},
{

View File

@ -2,4 +2,4 @@
@import "labels";
@import "list";
@import "task";
@import "tasks";
@import "tasks";

View File

@ -1,9 +1,9 @@
<template>
<div>
<message variant="success" class="has-text-centered" v-if="confirmedEmailSuccess">
<message variant="success" text-align="center" class="mb-4" v-if="confirmedEmailSuccess">
{{ $t('user.auth.confirmEmailSuccess') }}
konrad marked this conversation as resolved Outdated

has-text-centered should be a prop: e.g. text-align="center"

`has-text-centered` should be a prop: e.g. `text-align="center"`

What's the advatage of a prop when I could use a css class of the same length and readability?

What's the advatage of a prop when I could use a css class of the same length and readability?

Mostly: no unwanted side effects:

When you edit the component will you remember in half a year that you added a has-text-centered class with side effects from the outside somewhere in the app?

When you then change the inner layout of the component in a way that would just work with left-aligned text, that you assume, it will break.

By contrast:

If you do have a prop you do know exactly what different modes this component can have.

When you use something like storybook you can then also see and test a component in all different configurations (I don't mean automated tests, but that's also somehow possible).

Usually when I add classes to components I try to limit myself to change only CSS properties that change the outer alignment (stuff like margin, width, height, position of the component but not the inner apperance.

This article explains this in detail:

https://markus.oberlehner.net/blog/multiple-root-nodes-and-attribute-inheritance-in-vue-3/

(I also forgot the stuff with multiple root nodes)

Mostly: no unwanted side effects: When you edit the component will you remember in half a year that you added a has-text-centered class with side effects from the outside somewhere in the app? When you then change the inner layout of the component in a way that would just work with left-aligned text, that you assume, it will break. By contrast: If you do have a prop you do know exactly what different modes this component can have. When you use something like storybook you can then also see and test a component in all different configurations (I don't mean automated tests, but that's also somehow possible). Usually when I add classes to components I try to limit myself to change only CSS properties that change the outer alignment (stuff like margin, width, height, position of the component but not the inner apperance. This article explains this in detail: https://markus.oberlehner.net/blog/multiple-root-nodes-and-attribute-inheritance-in-vue-3/ (I also forgot the stuff with multiple root nodes)

That seems like a very good reason. I've added a prop.

That seems like a very good reason. I've added a prop.
</message>
<message variant="danger" v-if="errorMessage">
<message variant="danger" v-if="errorMessage" class="mb-4">
{{ errorMessage }}
</message>
<form @submit.prevent="submit" id="loginform" v-if="localAuthEnabled">
@ -20,24 +20,26 @@
autocomplete="username"
v-focus
@keyup.enter="submit"
konrad marked this conversation as resolved Outdated

AFAIK it's a tabindex with a higher index than 0 is usually not what you want.
I think that in this case you also probably don't want to assign a tabindex at all (since the input has one by default already).

Good writeup: https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex

Reminds me off: I always wanted to try:

EDIT:
Just saw that the reason for the numbering was the passwort reset link.
Nice! Seems like I learn something here =)

~~AFAIK it's a tabindex with a higher index than 0 is usually not what you want. I think that in this case you also probably don't want to assign a tabindex at all (since the input has one by default already).~~ ~~Good writeup: https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex~~ Reminds me off: I always wanted to try: - https://github.com/dequelabs/axe-core/ or - https://github.com/vue-a11y/vue-axe-next ...but I'm unsure if it's worth the effort and is even maintained (the latter). **EDIT:** Just saw that the reason for the numbering was the passwort reset link. Nice! Seems like I learn something here =)

Reminds me off: I always wanted to try:

https://github.com/dequelabs/axe-core/ or
https://github.com/vue-a11y/vue-axe-next
...but I'm unsure if it's worth the effort and is even maintained (the latter).

Yeah a11y is definitely something we should improve in Vikunja overall.

> Reminds me off: I always wanted to try: > > https://github.com/dequelabs/axe-core/ or > https://github.com/vue-a11y/vue-axe-next > ...but I'm unsure if it's worth the effort and is even maintained (the latter). Yeah a11y is definitely something we should improve in Vikunja overall.
tabindex="1"
@focusout="validateField('username')"
/>
</div>
<p class="help is-danger" v-if="!usernameValid">
{{ $t('user.auth.usernameRequired') }}
</p>
</div>
<div class="field">
<label class="label" for="password">{{ $t('user.auth.password') }}</label>
<div class="control">
<input
class="input"
id="password"
name="password"
:placeholder="$t('user.auth.passwordPlaceholder')"
ref="password"
required
type="password"
autocomplete="current-password"
@keyup.enter="submit"
/>
<div class="label-with-link">
<label class="label" for="password">{{ $t('user.auth.password') }}</label>
<router-link
:to="{ name: 'user.password-reset.request' }"
class="reset-password-link"
tabindex="6"
>
{{ $t('user.auth.forgotPassword') }}
</router-link>
</div>
<password tabindex="2" @submit="submit" v-model="password" :validate-initially="validatePasswordInitially"/>
</div>
konrad marked this conversation as resolved Outdated

Why is the input relative? Asking because the <a> is not inside the input. But might also miss something here.

Why is the input relative? Asking because the `<a>` is not inside the input. But might also miss something here.

That's a good catch. Removed it, wasn't actually used.

That's a good catch. Removed it, wasn't actually used.
<div class="field" v-if="needsTotpPasscode">
<label class="label" for="totpPasscode">{{ $t('user.auth.totpTitle') }}</label>
@ -52,32 +54,28 @@
type="text"
v-focus
konrad marked this conversation as resolved Outdated

Add aria-label and tooltip

Add aria-label and tooltip

Done.

Done.
@keyup.enter="submit"
tabindex="3"
/>
</div>
</div>
<div class="field is-grouped login-buttons">
<div class="control is-expanded">
<x-button
@click="submit"
:loading="loading"
>
{{ $t('user.auth.login') }}
</x-button>
<x-button
:to="{ name: 'user.register' }"
v-if="registrationEnabled"
variant="secondary"
>
{{ $t('user.auth.register') }}
</x-button>
</div>
<div class="control">
<router-link :to="{ name: 'user.password-reset.request' }" class="reset-password-link">
{{ $t('user.auth.forgotPassword') }}
</router-link>
</div>
</div>
<x-button
@click="submit"
:loading="loading"
tabindex="4"
>
{{ $t('user.auth.login') }}
</x-button>
<p class="mt-2" v-if="registrationEnabled">
{{ $t('user.auth.noAccountYet') }}
<router-link
:to="{ name: 'user.register' }"
type="secondary"
tabindex="5"
>
{{ $t('user.auth.createAccount') }}
</router-link>
</p>
</form>
<div
@ -97,6 +95,7 @@
</template>
<script>
import {useDebounceFn} from '@vueuse/core'
import {mapState} from 'vuex'
import {HTTPFactory} from '@/http-common'
@ -105,15 +104,20 @@ import {getErrorText} from '@/message'
import Message from '@/components/misc/message'
import {redirectToProvider} from '../../helpers/redirectToProvider'
import {getLastVisited, clearLastVisited} from '../../helpers/saveLastVisited'
konrad marked this conversation as resolved Outdated

What happens if neither hasApiUrl && localAuthEnabled nor hasOpenIdProviders is true?

What happens if neither `hasApiUrl && localAuthEnabled` nor `hasOpenIdProviders` is true?

The <ready> component wrapper will prompt the user to set an api url.

The `<ready>` component wrapper will prompt the user to set an api url.

Ahh for sure. Makes sense =)

Ahh for sure. Makes sense =)
import Password from '@/components/input/password'
export default {
components: {
Password,
Message,
},
data() {
return {
confirmedEmailSuccess: false,
errorMessage: '',
usernameValid: true,
password: '',
validatePasswordInitially: false,
}
},
beforeMount() {
@ -166,6 +170,13 @@ export default {
localAuthEnabled: state => state.config.auth.local.enabled,
openidConnect: state => state.config.auth.openidConnect,
}),
validateField() {
// using computed so that debounced function definition stays
return useDebounceFn((field) => {
this[`${field}Valid`] = this.$refs[field].value !== ''
}, 100)
},
},
methods: {
setLoading() {
@ -185,7 +196,14 @@ export default {
// For more info, see https://kolaente.dev/vikunja/frontend/issues/78
const credentials = {
username: this.$refs.username.value,
password: this.$refs.password.value,
password: this.password,
}
if (credentials.username === '' || credentials.password === '') {
// Trigger the validation error messages
this.validateField('username')
this.validatePasswordInitially = true
return
}
if (this.needsTotpPasscode) {
@ -196,7 +214,7 @@ export default {
await this.$store.dispatch('auth/login', credentials)
this.$store.commit('auth/needsTotpPasscode', false)
} catch (e) {
if (e.response && e.response.data.code === 1017 && !credentials.totpPasscode) {
if (e.response?.data.code === 1017 && !this.credentials.totpPasscode) {
return
}
@ -211,22 +229,21 @@ export default {
</script>
<style lang="scss" scoped>
.login-buttons {
@media screen and (max-width: 450px) {
flex-direction: column;
.control:first-child {
margin-bottom: 1rem;
}
}
}
.button {
margin: 0 0.4rem 0 0;
}
.reset-password-link {
display: inline-block;
padding-top: 5px;
}
.label-with-link {
konrad marked this conversation as resolved Outdated

e.response?.data.code === 1017 && !this.credentials.totpPasscode
null can never be 1017

`e.response?.data.code === 1017 && !this.credentials.totpPasscode` `null` can never be `1017`

Done.

Done.
display: flex;
justify-content: space-between;
margin-bottom: .5rem;
.label {
margin-bottom: 0;
}
}
</style>

View File

@ -1,9 +1,9 @@
<template>
<div>
<message v-if="errorMsg">
<message v-if="errorMsg" class="mb-4">
{{ errorMsg }}
</message>
<div class="has-text-centered" v-if="successMessage">
<div class="has-text-centered mb-4" v-if="successMessage">
<message variant="success">
{{ successMessage }}
</message>

View File

@ -1,6 +1,6 @@
<template>
<div>
<message variant="danger" v-if="errorMessage !== ''">
<message variant="danger" v-if="errorMessage !== ''" class="mb-4">
{{ errorMessage }}
</message>
<form @submit.prevent="submit" id="registerform">
@ -18,8 +18,12 @@
v-focus
v-model="credentials.username"
@keyup.enter="submit"
@focusout="validateUsername"
/>
</div>
<p class="help is-danger" v-if="!usernameValid">
{{ $t('user.auth.usernameRequired') }}
</p>
</div>
<div class="field">
<label class="label" for="email">{{ $t('user.auth.email') }}</label>
@ -33,68 +37,46 @@
type="email"
v-model="credentials.email"
@keyup.enter="submit"
@focusout="validateEmail"
/>
</div>
<p class="help is-danger" v-if="!emailValid">
{{ $t('user.auth.emailInvalid') }}
</p>
</div>
<div class="field">
<label class="label" for="password">{{ $t('user.auth.password') }}</label>
<div class="control">
<input
class="input"
id="password"
name="password"
:placeholder="$t('user.auth.passwordPlaceholder')"
required
type="password"
autocomplete="new-password"
v-model="credentials.password"
@keyup.enter="submit"
/>
</div>
</div>
<div class="field">
<label class="label" for="passwordValidation">{{ $t('user.auth.passwordRepeat') }}</label>
<div class="control">
<input
class="input"
id="passwordValidation"
name="passwordValidation"
:placeholder="$t('user.auth.passwordPlaceholder')"
required
type="password"
autocomplete="new-password"
v-model="passwordValidation"
@keyup.enter="submit"
/>
</div>
<password @submit="submit" @update:modelValue="v => credentials.password = v" :validate-initially="validatePasswordInitially"/>
</div>
<div class="field is-grouped">
<div class="control">
<x-button
:loading="loading"
id="register-submit"
@click="submit"
class="mr-2"
>
{{ $t('user.auth.register') }}
</x-button>
<x-button :to="{ name: 'user.login' }" variant="secondary">
{{ $t('user.auth.login') }}
</x-button>
</div>
</div>
<x-button
:loading="loading"
id="register-submit"
@click="submit"
class="mr-2"
:disabled="!everythingValid"
>
{{ $t('user.auth.createAccount') }}
</x-button>
<p class="mt-2">
{{ $t('user.auth.alreadyHaveAnAccount') }}
<router-link :to="{ name: 'user.login' }">
{{ $t('user.auth.login') }}
</router-link>
</p>
</form>
</div>
</template>
<script setup>
import {useDebounceFn} from '@vueuse/core'
import {ref, reactive, toRaw, computed, onBeforeMount} from 'vue'
import {useI18n} from 'vue-i18n'
import router from '@/router'
import {store} from '@/store'
import Message from '@/components/misc/message'
import {isEmail} from '@/helpers/isEmail'
import Password from '@/components/input/password'
// FIXME: use the `beforeEnter` hook of vue-router
// Check if the user is already logged in, if so, redirect them to the homepage
@ -104,27 +86,45 @@ onBeforeMount(() => {
}
})
const {t} = useI18n()
const credentials = reactive({
username: '',
email: '',
password: '',
})
const passwordValidation = ref('')
const loading = computed(() => store.state.loading)
const errorMessage = ref('')
const validatePasswordInitially = ref(false)
const DEBOUNCE_TIME = 100
// debouncing to prevent error messages when clicking on the log in button
const emailValid = ref(true)
const validateEmail = useDebounceFn(() => {
emailValid.value = isEmail(credentials.email)
}, DEBOUNCE_TIME)
const usernameValid = ref(true)
const validateUsername = useDebounceFn(() => {
usernameValid.value = credentials.username !== ''
}, DEBOUNCE_TIME)
const everythingValid = computed(() => {
return credentials.username !== '' &&
credentials.email !== '' &&
credentials.password !== '' &&
konrad marked this conversation as resolved Outdated

I want to get rid of my useTitle function. The vueuse version seems much better.
There you can write:

import {useTitle} from '@vueuse/core'
useTitle(t('user.auth.createAccount'))
I want to get rid of my `useTitle` function. The vueuse version seems much better. There you can write: ```ts import {useTitle} from '@vueuse/core' useTitle(t('user.auth.createAccount')) ```

But Vikunja's setTitle also appends the | Vikunja part to the page title. vueuse does not do that.

Maybe we could change our useTitle wrapper to use Vikunja's setTitle function? I think this would be out of scope for this PR though.

But Vikunja's `setTitle` also appends the ` | Vikunja` part to the page title. vueuse does not do that. Maybe we could change our `useTitle` wrapper to use Vikunja's `setTitle` function? I think this would be out of scope for this PR though.
It does already: https://kolaente.dev/vikunja/frontend/src/branch/main/src/composables/useTitle.ts#L2 ?

But vueuse's useTitle does not.

But vueuse's `useTitle` does not.

Ahh yes. So you mean we create a wrapper for vueuse useTitle that does the same as setTitle?

Ahh yes. So you mean we create a wrapper for vueuse useTitle that does the same as setTitle?

yeah exactly.

yeah exactly.
emailValid.value &&
usernameValid.value
})
async function submit() {
errorMessage.value = ''
validatePasswordInitially.value = true
if (credentials.password !== passwordValidation.value) {
errorMessage.value = t('user.auth.passwordsDontMatch')
if (!everythingValid.value) {
return
}
konrad marked this conversation as resolved Outdated

I did not get this.

I did not get this.
try {
await store.dispatch('auth/register', toRaw(credentials))
} catch (e) {

View File

@ -1,9 +1,9 @@
<template>
<div>
<message variant="danger" v-if="errorMsg">
<message variant="danger" v-if="errorMsg" class="mb-4">
{{ errorMsg }}
</message>
<div class="has-text-centered" v-if="isSuccess">
<div class="has-text-centered mb-4" v-if="isSuccess">
<message variant="success">
{{ $t('user.auth.resetPasswordSuccess') }}
</message>