feat: use script setup and ts in app auth components #1160
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#1160
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "dpschen/frontend:feature/feat-ts-and-setup-in-app-auth"
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?
074e3713d1
tob833555511
Hi dpschen!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1160-featurefeat-ts-and-setup-in-app-auth--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!
WIP: feat: use script setup and ts in app auth componentsto feat: use script setup and ts in app auth componentsLooks good, only some nits.
@ -24,0 +21,4 @@
import {useRoute, useRouter} from 'vue-router'
import {useStore} from 'vuex'
import {useI18n} from 'vue-i18n'
import { useOnline } from '@vueuse/core'
Please make it consistent with the rest:
I kind of ignored this kind of formatting for now since this can be automated with prettier / eslint.
That's fine too. Another reason to integrate prettier :)
Changed it anyway for now
@ -113,0 +47,4 @@
const router = useRouter()
// Make sure to always load the home route when running with electron
if (route.fullPath.endsWith('frontend/index.html')) {
I'm not even sure if we need this anymore. Will check.
We definitely don't need this. Could you remove it?
Removed
@ -132,0 +86,4 @@
// TODO: Reset the title if the page component does not set one itself
function useRenewTokenOnFocus() {
This looks like an excellent candidate to be moved in a seperate file and imported from there. Could be another PR though.
Yes for example.
I wanted to move the files to script setup because this makes refactoring easier =)
So this does for sure not solve everything for auth
b833555511
toc89ceb7e38
c89ceb7e38
to2643b62fd7
@ -122,0 +55,4 @@
const {t} = useI18n()
// setup account deletion verification
const accountDeletionConfirm = useRouteQuery('accountDeletionConfirm') as Ref<null | string>
Not sure about this typing 🤷♂️ but seems to work.
2643b62fd7
to3813a0c388
@ -23,6 +23,7 @@
"@sentry/vue": "6.16.1",
"@vue/compat": "3.2.26",
"@vueuse/core": "7.3.0",
"@vueuse/router": "^7.3.0",
This package is super slim so I felt there is no problem to add. Would have written something similar but probably worse either way.
yeah should be fine.
@ -123,0 +56,4 @@
// setup account deletion verification
const accountDeletionConfirm = useRouteQuery('accountDeletionConfirm') as Ref<null | string>
watch(accountDeletionConfirm, async (accountDeletionConfirm) => {
When converting these functions to watchers in
807fb6a9fe
the initial call got removed. I readded it by addingimmediate: true
.I hope this was correct.
I only added the watcher because the initial call did not work at all. If your solution works without a watcher that should be fine.
It is still a watcher but just calls on initial setup. Didn't test if it works though, but I don't know why it shouldn't.
Just tested it, works great.
@konrad Can you recheck this?
@ -46,0 +14,4 @@
const route = useRoute()
watchEffect(() => {
Doesn't this need
immediate: true
as well?watchEffect runs immediate by default
@ -98,0 +112,4 @@
errorMessage,
password,
authenticate,
} = useAuth()
Any special reason to put this in a function and creating const from it instead of not using a function and declaring the variables directly?
😆 I also felt that this is very verbose.
The reason was that I wanted to prepare abstraction of the authentification part.
Since this pull request is mostly about converting to script setup + ts this is probably one little extra setup too much (the function wrapper).
That makes sense.
LGTM, just waiting for the CI to pass.
🥳