feat: defer everything until the api config is loaded #926

Merged
konrad merged 27 commits from feature/ready-state into main 2021-11-13 19:49:03 +00:00
5 changed files with 122 additions and 67 deletions
Showing only changes of commit e2cc505564 - Show all commits

View File

@ -1,40 +1,20 @@
<template>
<div class="no-auth-wrapper">
<div class="noauth-container">
<img alt="Vikunja" :src="logoUrl" width="400" height="117" />
<div class="message is-info" v-if="motd !== ''">
<div class="message-header">
<p>{{ $t('misc.info') }}</p>
</div>
<div class="message-body">
{{ motd }}
</div>
</div>
<router-view/>
</div>
</div>
<no-auth-wrapper>
<router-view/>
</no-auth-wrapper>
</template>
<script>
import {mapState} from 'vuex'
import logoUrl from '@/assets/logo-full.svg'
import { saveLastVisited } from '@/helpers/saveLastVisited'
import {saveLastVisited} from '@/helpers/saveLastVisited'
import NoAuthWrapper from '@/components/misc/no-auth-wrapper'
export default {
name: 'contentNoAuth',
data() {
return {
logoUrl,
}
},
components: {NoAuthWrapper},
computed: {
routeName() {
return this.$route.name
},
...mapState({
motd: state => state.config.motd,
}),
},
watch: {
routeName: {
@ -65,17 +45,3 @@ export default {
},
}
</script>
<style lang="scss" scoped>
.no-auth-wrapper {
background: url('@/assets/llama.svg') no-repeat bottom left fixed $light-background;
min-height: 100vh;
}
.noauth-container {
max-width: 450px;
width: 100%;
margin: 0 auto;
padding: 1rem;
}
</style>

View File

@ -70,30 +70,45 @@ export default {
return parseURL(this.apiUrl).host
},
},
props: {
configureOpen: {
dpschen marked this conversation as resolved
Review

I was first confused by the naming of this prop.
Since the component is just use once and if used we set this to true:
shall we just remove it to simplify this?

I was first confused by the naming of this prop. Since the component is just use once and if used we set this to true: shall we just remove it to simplify this?
Review

The <api-config> component is used in two places (ready and login view) and this prop is only used in one of those. Just removing the prop does not seem to be a good option here.

How would you simplify it?

The `<api-config>` component is used in two places (ready and login view) and this prop is only used in one of those. Just removing the prop does not seem to be a good option here. How would you simplify it?
Review

Can you explain why api-config is also in Login.vue? I think I didn't get that =)

Can you explain why api-config is also in Login.vue? I think I didn't get that =)
Review

Mostly to be able to change the url on the login screen, even if one is already defined. It also shows the user what api server they are connecting to.

The login screen is the first entry point, but really this should be on the other noauth screens as well (Register, Password reset etc).

I'm planning a follow-up PR to refactor the whole noauth thing a bit, will include that there.

Mostly to be able to change the url on the login screen, even if one is already defined. It also shows the user what api server they are connecting to. The login screen is the first entry point, but really this should be on the other `noauth` screens as well (Register, Password reset etc). I'm planning a follow-up PR to refactor the whole noauth thing a bit, will include that there.
Review

I'm a bit afraid that I'll never be able to merge the modals branch #816 if we don't plan ahead.

Maybe we can align those changes you still want to do or built them after merge on top of the modals branch.

I'm a bit afraid that I'll never be able to merge the modals branch https://kolaente.dev/vikunja/frontend/pulls/816 if we don't plan ahead. Maybe we can align those changes you still want to do or built them after merge on top of the modals branch.
Review

Sure. It should be fine to do them after the modals branch is done.

Sure. It should be fine to do them after the modals branch is done.
Review

Let's resolve this and merge this branch. Then I can try to merge it into #816 and we can think about how to continue =)

Let's resolve this and merge this branch. Then I can try to merge it into #816 and we can think about how to continue =)
type: Boolean,
required: false,
default: false,
},
},
watch: {
configureOpen: {
handler(value) {
this.configureApi = value
},
immediate: true,
},
},
methods: {
setApiUrl() {
async setApiUrl() {
if (this.apiUrl === '') {
return
konrad marked this conversation as resolved Outdated

Explain why we return here

Explain why we return here

Done.

Done.
}
konrad marked this conversation as resolved Outdated

That seems like it should throw an error?

That seems like it should throw an error?

It does yeah. I've modified it so it shows an error with a "good" error message to the user instead of throwing one.

It does yeah. I've modified it so it shows an error with a "good" error message to the user instead of throwing one.
checkAndSetApiUrl(this.apiUrl, () => this.$store.dispatch('config/update'))
.catch(() => {
// Still not found, url is still invalid
this.successMsg = ''
this.errorMsg = this.$t('apiConfig.error', {domain: this.apiDomain})
})
.then(url => {
if (url === '') {
return
}
try {
const url = await checkAndSetApiUrl(this.apiUrl, () => this.$store.dispatch('config/update'))
// Set it + save it to local storage to save us the hoops
this.errorMsg = ''
this.successMsg = this.$t('apiConfig.success', {domain: this.apiDomain})
this.configureApi = false
this.apiUrl = url
this.$emit('foundApi', this.apiUrl)
})
if (url === '') {
return
konrad marked this conversation as resolved
Review

Explain why we return here

Explain why we return here
}
konrad marked this conversation as resolved Outdated

That seems like it should throw an error? (2)

That seems like it should throw an error? (2)
// Set it + save it to local storage to save us the hoops
this.errorMsg = ''
this.successMsg = this.$t('apiConfig.success', {domain: this.apiDomain})
this.configureApi = false
this.apiUrl = url
this.$emit('foundApi', this.apiUrl)
} catch (e) {
// Still not found, url is still invalid
this.successMsg = ''
this.errorMsg = this.$t('apiConfig.error', {domain: this.apiDomain})
}
},
},
}

View File

@ -0,0 +1,47 @@
<template>
<div class="no-auth-wrapper">
<div class="noauth-container">
<img alt="Vikunja" :src="logoUrl" width="400" height="117"/>
<div class="message is-info" v-if="motd !== ''">
<div class="message-header">
<p>{{ $t('misc.info') }}</p>
</div>
<div class="message-body">
{{ motd }}
</div>
</div>
<slot/>
</div>
</div>
</template>
<script>
konrad marked this conversation as resolved Outdated

Use script setup

Use script setup

Makes a lot of sense. Done!

Makes a lot of sense. Done!
import logoUrl from '@/assets/logo-full.svg'
import {mapState} from 'vuex'
export default {
name: 'no-auth-wrapper',
data() {
return {
logoUrl,
}
},
computed: mapState({
motd: state => state.config.motd,
}),
}
</script>
<style lang="scss" scoped>
.no-auth-wrapper {
background: url('@/assets/llama.svg') no-repeat bottom left fixed $light-background;
min-height: 100vh;
konrad marked this conversation as resolved
Review

The min-height should be set from outside.

The min-height should be set from outside.
Review

And the background as well? Because the background will only work with a height of 100vh (to keep the llama at the bottom).

And the background as well? Because the background will only work with a height of 100vh (to keep the llama at the bottom).
Review

Would probably be best to import the llama image as component with the new vite-svg-loader in the parent component.
Then it can be positioned absolute.
Maybe add a new issue and we resolve that after merging. Because there will be conflicts and I think that's hard to solve before.

Would probably be best to import the llama image as component with the new vite-svg-loader in the parent component. Then it can be positioned absolute. Maybe add a new issue and we resolve that after merging. Because there will be conflicts and I think that's hard to solve before.
Review

Opened: #973

Opened: https://kolaente.dev/vikunja/frontend/issues/973
}
.noauth-container {
max-width: 450px;
width: 100%;
margin: 0 auto;
padding: 1rem;
}
</style>

View File

@ -3,23 +3,41 @@
<slot/>
</template>
<section v-else-if="error !== ''">
{{ error }}
<no-auth-wrapper>
<card>
<div class="notification is-danger">
<p>
{{ $t('ready.errorOccured') }}<br/>
{{ error }}
</p>
<p>
{{ $t('ready.checkApiUrl') }}
</p>
</div>
<api-config :configure-open="true" @found-api="load"/>
</card>
</no-auth-wrapper>
</section>
<section class="vikunja-loading" v-else>
<img alt="Vikunja" :src="logoUrl" width="100" height="100"/>
<p>
<span class="loader-container is-loading-small is-loading"></span>
{{ $t('home.vikunjaLoading') }}
{{ $t('ready.loading') }}
</p>
</section>
</template>
<script>
import logoUrl from '@/assets/logo.svg'
import {setLanguage} from '@/i18n'
import ApiConfig from '@/components/misc/api-config'
import NoAuthWrapper from '@/components/misc/no-auth-wrapper'
export default {
name: 'ready',
components: {
NoAuthWrapper,
ApiConfig,
},
data() {
return {
logoUrl,
@ -27,16 +45,21 @@ export default {
}
},
mounted() {
this.$store.dispatch('loadApp')
.catch(e => {
this.error = e
})
this.load()
},
computed: {
ready() {
return this.$store.state.vikunjaReady
},
},
methods: {
konrad marked this conversation as resolved Outdated

start loading in created

start loading in created

Done.

Done.
load() {
this.$store.dispatch('loadApp')
.catch(e => {
this.error = e
})
},
},
}
</script>

View File

@ -1,6 +1,5 @@
{
"home": {
"vikunjaLoading": "Vikunja is loading…",
"welcomeNight": "Good Night {username}",
"welcomeMorning": "Good Morning {username}",
"welcomeDay": "Hi {username}",
@ -17,6 +16,11 @@
"title": "Not found",
"text": "The page you requested does not exist."
},
"ready": {
"loading": "Vikunja is loading…",
"errorOccured": "An error occured:",
"checkApiUrl": "Please check if the api url is correct below."
},
"user": {
"auth": {
"username": "Username",
@ -777,7 +781,7 @@
"urlPlaceholder": "eg. https://localhost:3456",
"change": "change",
"signInOn": "Sign in to your Vikunja account on {0}",
"error": "Could not find or use Vikunja installation at \"{domain}\".",
"error": "Could not find or use Vikunja installation at \"{domain}\". Please try a different url.",
"success": "Using Vikunja installation at \"{domain}\"."
},
"loadingError": {