feat: improve login and register ux #1104
No reviewers
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
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#1104
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature/login-improvements"
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 adds error messages beneath the input fields they correspond to and removes the need for entering the password twice.
Depends on #1103
feat: improve login and register uxto WIP: feat: improve login and register ux@ -0,0 +1,12 @@
export function isValidEmail(email: string): Boolean {
As far as I know it's not possible to validate emails using regex.
Last time I checked best practise was: check if there is a
@
symbol somewhere (!) and if yes:see it as valid, but don't use it until the user clicks on a email verification link.
Changed it so the validation only looks for an
@
. There's additional validation on the api anyway, this is only used for frontend niceties.What kind of validation do you use then on the api side?
Because there obviously the same applies.
This one:
f21760c49a/patterns.go (L7)
(with a package)Looks slightly more complex than what I'm doing in the frontend.
Pretty sure this has the same issues.
Probably yeah. Since it is quite a popular package I'd assume it will validate 99.999% of correct email as correct which should be enough for Vikunja's use case.
@ -0,0 +2,4 @@
const format = /^\w+([\.-]?\w+)*@\w+([\.-]?\w+)*(\.\w{2,3})+$/
const match = email.match(format)
console.log(match)
Remove
console.log
@ -45,3 +45,3 @@
"totpPlaceholder": "e.g. 123456",
"login": "Login",
"register": "Register",
"createAccount": "Create account",
This is much better! =)
@ -51,2 +51,3 @@
"openIdGeneralError": "An error occured while authenticating against the third party.",
"logout": "Logout"
"logout": "Logout",
"emailInvalid": "Please enter a valid email address.",
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.
@ -23,2 +3,2 @@
/>
</div>
<h2 class="title">Login</h2>
<message variant="success" class="has-text-centered" v-if="confirmedEmailSuccess">
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?
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.
@ -25,0 +19,4 @@
autocomplete="username"
v-focus
@keyup.enter="submit"
tabindex="1"
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-tabindexReminds me off: I always wanted to try:
...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 =)
Yeah a11y is definitely something we should improve in Vikunja overall.
@ -56,1 +41,3 @@
</div>
<div class="control">
<input
class="input is-relative"
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.
@ -57,0 +52,4 @@
tabindex="2"
@focusout="validateField('password')"
/>
<a @click="togglePasswordFieldType" class="password-field-type-toggle">
Add aria-label and tooltip
Done.
@ -99,2 +104,3 @@
<legal/>
<div
v-if="hasOpenIdProviders"
What happens if neither
hasApiUrl && localAuthEnabled
norhasOpenIdProviders
is true?The
<ready>
component wrapper will prompt the user to set an api url.Ahh for sure. Makes sense =)
@ -210,3 +238,3 @@
this.$store.commit('auth/needsTotpPasscode', false)
} catch (e) {
if (e.response && e.response.data.code === 1017 && !credentials.totpPasscode) {
if (e.response && e.response.data.code === 1017 && !this.credentials.totpPasscode) {
e.response?.data.code === 1017 && !this.credentials.totpPasscode
null
can never be1017
Done.
@ -246,0 +284,4 @@
.password-field-type-toggle {
position: absolute;
top: .5rem;
Seems error prone, because vertical center is estimated. Maybe better:
Better: abstract this as a component. Then you don't have to repeat this.
I've moved this to a style tag in a new scss file instead. While still not ideal, it avoids repeating the css class.
I'd need to move the input in a new combined component as well. But because we're not using
v-model
to get the actual password here this would involve rebuilding quite a few events. Not sure if that's worth it? Open for ideas.Might be worth it:
We can use the same field for the password settings.
Also you don't have to repeat the logic for the password label for the password hide and show that is now repeated in two components (and should probably be the same in password settings).
Moved it to a new component, probably not a perfect implementation though (see other comment).
@ -115,3 +113,3 @@
const {t} = useI18n()
useTitle(() => t('user.auth.register'))
useTitle(() => t('user.auth.createAccount'))
I want to get rid of my
useTitle
function. The vueuse version seems much better.There you can write:
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'ssetTitle
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.Ahh yes. So you mean we create a wrapper for vueuse useTitle that does the same as setTitle?
yeah exactly.
@ -127,2 +124,4 @@
const errorMessage = ref('')
const emailValid = ref(true)
const validateEmail = useDebounceFn(() => { // debouncing to prevent error messages when clicking on the log in button
I did not get this.
@ -129,0 +139,4 @@
}, 100)
const passwordFieldType = ref('password')
const togglePasswordFieldType = () => {
if (passwordFieldType.value === 'password') {
Done.
@ -129,0 +146,4 @@
}
}
const everythingValid = computed(() => {
For this limited usecase this seems fine, but in case we do something similar again we should probably abstract this a bit nicer.
This seems like a nice usecase for a
reactive
.If all the form fields would store there validation value there you could do something like:
@ -131,3 +160,2 @@
if (credentials.password !== passwordValidation.value) {
errorMessage.value = t('user.auth.passwordsDontMatch')
if (!everythingValid) {
everythingValid.value
@ -145,0 +173,4 @@
<style scoped>
.password-field-type-toggle {
position: absolute;
top: .5rem;
See comment for other
.password-field-type-toggle
@ -61,3 +56,3 @@
const { t } = useI18n()
const {t} = useI18n()
useTitle(() => t('user.auth.resetPassword'))
Use
useTitle
from vueuseRelevant: #1103 (comment)
About the debounce: Check out the screen recording in the attachment to see what it does without that.
Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1104-featurelogin-improvements--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.
Have a nice day!
@ -7,2 +5,2 @@
</Message>
<slot/>
<section class="image" :class="{'has-message': motd !== ''}">
<Message v-if="motd !== ''">
I just realized that this is not shown on mobile. Is that intended?
It was not. I've added it back in on mobile.
201ec53fcc
to507ef8a7ef
WIP: feat: improve login and register uxto feat: improve login and register uxbd579565b7
to22d209b289
22d209b289
toa4ec41e937
@ -29,3 +31,4 @@
import Legal from '@/components/misc/legal.vue'
import ApiConfig from '@/components/misc/api-config.vue'
import {useStore} from 'vuex'
import {computed} from 'vue'
Regarding (not allowed to comment in unchanged lines in Gitea):
Why
@ts-ignore
?Fixed
@ -0,0 +34,4 @@
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,
I'm not quite sure if this is the best way to do it. Feels a lot like a hack.