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

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
Conversation 3 Commits 27 Files Changed 10 +419 -255
konrad commented 2021-10-31 20:52:07 +00:00
Owner
Copy Link

This PR adds a wrapper component in App.vue that checks if a connection to the api can be made successfully. If that fails, it shows a dialog to configure the api url.

This should allow us to ensure all api-related config (from /info) is fully loaded and ready to use, preventing race-conditions and hacky double checks of settings.

This PR adds a wrapper component in `App.vue` that checks if a connection to the api can be made successfully. If that fails, it shows a dialog to configure the api url. This should allow us to ensure all api-related config (from `/info`) is fully loaded and ready to use, preventing race-conditions and hacky double checks of settings.
konrad added 4 commits 2021-10-31 20:52:07 +00:00
continuous-integration/drone/pr Build is failing Details
acc315b581
feat: add fade to loading spinner
konrad added 1 commit 2021-10-31 21:00:58 +00:00
continuous-integration/drone/pr Build is failing Details
a7a7c75dc5
feat: check for empty url
konrad added 1 commit 2021-10-31 21:01:36 +00:00
continuous-integration/drone/pr Build is passing Details
8edae1114f
fix: lint
konrad requested review from dpschen 2021-10-31 21:01:42 +00:00
konrad commented 2021-10-31 21:31:17 +00:00
Author
Owner
Copy Link

Maybe we should integrate this here as well? That would enable redirecting the user without showing a button or other UI.

Maybe it would be enough to never set the ready state to true until redirected though.

Maybe we should integrate [this](https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/home/contentNoAuth.vue#L49) here as well? That would enable redirecting the user without showing a button or other UI. Maybe it would be enough to never set the ready state to `true` until redirected though.
dpschen self-assigned this 2021-11-01 00:39:41 +00:00
dpschen reviewed 2021-11-04 15:28:40 +00:00
src/App.vue Outdated
@ -135,3 +128,1 @@
bottom: 5vh;
color: $white;
padding: 0 1rem;
.offline-message {
dpschen commented 2021-11-04 15:28:40 +00:00
Member

unnest

unnest
konrad commented 2021-11-10 18:52:04 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:31:39 +00:00
src/App.vue Outdated
@ -16,0 +4,4 @@
<div :class="{'is-hidden': !online}">
<!-- This is a workaround to get the sw to "see" the to-be-cached version of the offline background image -->
<div class="offline" style="height: 0;width: 0;"></div>
<top-navigation v-if="authUser"/>
dpschen commented 2021-11-04 15:31:39 +00:00
Member

Wrap in <template v-if="authUser">

Wrap in `<template v-if="authUser">`
konrad commented 2021-11-10 18:52:17 +00:00
Author
Owner

Why?

Why?
dpschen commented 2021-11-10 19:06:03 +00:00
Member

to make clear that it mounts at the same time than the content-auth below.

to make clear that it mounts at the same time than the content-auth below.
konrad commented 2021-11-10 19:16:53 +00:00
Author
Owner

Ah, that makes sense. Done.

Ah, that makes sense. Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:33:38 +00:00
src/components/misc/api-config.vue Outdated
@ -50,2 +49,2 @@
const API_DEFAULT_PORT = 3456
import {parseURL} from 'ufo'
import {checkAndSetApiUrl} from '../../helpers/checkAndSetApiUrl'
dpschen commented 2021-11-04 15:33:38 +00:00
Member

use @

use `@`
konrad commented 2021-11-10 18:52:39 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:38:25 +00:00
src/components/misc/api-config.vue Outdated
@ -210,3 +126,1 @@
span.url {
border-bottom: 1px dashed $primary;
}
span.url {
dpschen commented 2021-11-04 15:38:25 +00:00
Member
  • unnest
  • try to not use a tag selector: does using just .url work? if not -> How about .api-url
- unnest - try to not use a tag selector: does using just `.url` work? if not -> How about `.api-url`
konrad commented 2021-11-10 18:54:29 +00:00
Author
Owner

The .url isn't used anywhere else. Unnesting and omitting the tag seems to work.

The `.url` isn't used anywhere else. Unnesting and omitting the tag seems to work.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:38:58 +00:00
src/components/misc/no-auth-wrapper.vue
@ -0,0 +35,4 @@
<style lang="scss" scoped>
.no-auth-wrapper {
background: url('@/assets/llama.svg') no-repeat bottom left fixed $light-background;
min-height: 100vh;
dpschen commented 2021-11-04 15:38:58 +00:00
Member

The min-height should be set from outside.

The min-height should be set from outside.
konrad commented 2021-11-10 18:55:33 +00:00
Author
Owner

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).
dpschen commented 2021-11-10 20:47:28 +00:00
Member

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.
konrad commented 2021-11-10 21:05:47 +00:00
Author
Owner

Opened: #973

Opened: https://kolaente.dev/vikunja/frontend/issues/973
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:44:26 +00:00
src/components/misc/api-config.vue
@ -72,2 +71,4 @@
},
},
props: {
configureOpen: {
dpschen commented 2021-11-04 15:44:26 +00:00
Member

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?
konrad commented 2021-11-10 18:58:18 +00:00
Author
Owner

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?
dpschen commented 2021-11-10 20:59:22 +00:00
Member

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 =)
konrad commented 2021-11-10 21:03:07 +00:00
Author
Owner

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.
dpschen commented 2021-11-12 17:41:59 +00:00
Member

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.
konrad commented 2021-11-13 14:47:37 +00:00
Author
Owner

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.
dpschen commented 2021-11-13 19:29:16 +00:00
Member

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 =)
👍 1
dpschen marked this conversation as resolved
dpschen reviewed 2021-11-04 15:44:56 +00:00
src/components/misc/api-config.vue Outdated
@ -75,3 +89,3 @@
setApiUrl() {
async setApiUrl() {
if (this.apiUrl === '') {
return
dpschen commented 2021-11-04 15:44:56 +00:00
Member

Explain why we return here

Explain why we return here
konrad commented 2021-11-10 19:01:43 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:45:07 +00:00
src/components/misc/api-config.vue
@ -90,2 +97,2 @@
urlToCheck = new URL(urlToCheck)
const origUrlToCheck = urlToCheck
if (url === '') {
return
dpschen commented 2021-11-04 15:45:07 +00:00
Member

Explain why we return here

Explain why we return here
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:46:29 +00:00
src/components/misc/no-auth-wrapper.vue Outdated
@ -0,0 +15,4 @@
</div>
</template>
<script>
dpschen commented 2021-11-04 15:46:29 +00:00
Member

Use script setup

Use script setup
konrad commented 2021-11-10 19:08:03 +00:00
Author
Owner

Makes a lot of sense. Done!

Makes a lot of sense. Done!
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:47:07 +00:00
src/components/misc/ready.vue Outdated
@ -0,0 +52,4 @@
errorNoApiUrl: ERROR_NO_API_URL,
}
},
mounted() {
dpschen commented 2021-11-04 15:47:07 +00:00
Member

start loading in created

start loading in created
konrad commented 2021-11-10 19:08:42 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:48:17 +00:00
src/components/misc/ready.vue Outdated
@ -0,0 +99,4 @@
margin-bottom: 1rem;
}
.loader-container {
dpschen commented 2021-11-04 15:48:17 +00:00
Member

unnest loader-container

unnest loader-container
konrad commented 2021-11-10 19:09:22 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:48:46 +00:00
src/components/misc/ready.vue Outdated
@ -0,0 +102,4 @@
.loader-container {
margin-right: 1rem;
&.is-loading:after {
dpschen commented 2021-11-04 15:48:46 +00:00
Member

&.is-loading::after { (double ::)

`&.is-loading::after {` (double `::`)
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:49:31 +00:00
src/store/index.js Outdated
@ -19,6 +19,7 @@ import attachments from './modules/attachments'
import labels from './modules/labels'
import ListService from '../services/list'
import {checkAndSetApiUrl} from '../helpers/checkAndSetApiUrl'
dpschen commented 2021-11-04 15:49:31 +00:00
Member

Use @

Use `@`
konrad commented 2021-11-10 19:09:55 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:53:04 +00:00
src/styles/theme/loading.scss Outdated
@ -3,3 +2,1 @@
position: relative;
pointer-events: none;
opacity: 0.5;
.loader-container {
dpschen commented 2021-11-04 15:53:04 +00:00
Member

how about:

// FIXME: move to loading.vue 
.loader-container.is-loading {
    position: relative;
    pointer-events: none;
    opacity: 0.5;

    &::after {
        @include loader;
        position: absolute;
        top: calc(50% - 2.5rem);
        left: calc(50% - 2.5rem);
        width: 5rem;
        height: 5rem;
        border-width: 0.25rem;
    }
    &.is-loading-small::after {
        width: 1.5rem;
        height: 1.5rem;
        top: calc(50% - .75rem);
        left: calc(50% - .75rem);
        border-width: 2px;
    }
}
how about: ```scss // FIXME: move to loading.vue .loader-container.is-loading { position: relative; pointer-events: none; opacity: 0.5; &::after { @include loader; position: absolute; top: calc(50% - 2.5rem); left: calc(50% - 2.5rem); width: 5rem; height: 5rem; border-width: 0.25rem; } &.is-loading-small::after { width: 1.5rem; height: 1.5rem; top: calc(50% - .75rem); left: calc(50% - .75rem); border-width: 2px; } } ```
konrad commented 2021-11-10 19:11:26 +00:00
Author
Owner

Makes sense. Done.

Makes sense. Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 15:59:15 +00:00
src/App.vue Outdated
@ -16,0 +10,4 @@
<content-no-auth v-else/>
<notification/>
</div>
<div class="app offline" v-if="!online">
dpschen commented 2021-11-04 15:59:15 +00:00
Member

Didn't test, but just from reading:
might it be that the not offline check needs to be outside of <ready>?

Else the api might not be reached because we are offline. => slot is never exposed.

Didn't test, but just from reading: might it be that the not offline check needs to be outside of `<ready>`? Else the api might not be reached because we are offline. => slot is never exposed.
konrad commented 2021-11-10 18:51:25 +00:00
Author
Owner

I think that's pretty much the case. Moved the offline overlay to the ready component.

I think that's pretty much the case. Moved the offline overlay to the ready component.
konrad marked this conversation as resolved
dpschen commented 2021-11-04 16:06:59 +00:00
Member
Copy Link

Maybe we should integrate this here as well? That would enable redirecting the user without showing a button or other UI.

Maybe it would be enough to never set the ready state to true until redirected though.

I wanted to build this logic in the router via route meta fields.

There is already a super wip implementation in:
0c7df1a927
Not working right now. I basically just dumped that code in there.

> Maybe we should integrate [this](https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/home/contentNoAuth.vue#L49) here as well? That would enable redirecting the user without showing a button or other UI. > > Maybe it would be enough to never set the ready state to `true` until redirected though. I wanted to build this logic in the router via [route meta fields](https://next.router.vuejs.org/guide/advanced/meta.html#route-meta-fields). There is already a super wip implementation in: https://kolaente.dev/vikunja/frontend/commit/0c7df1a927549170d761cbaef515e88d28d97a34 Not working right now. I basically just dumped that code in there.
👍 1
dpschen reviewed 2021-11-04 16:10:47 +00:00
src/components/misc/ready.vue Outdated
@ -0,0 +65,4 @@
},
methods: {
load() {
if (window.API_URL === '') {
dpschen commented 2021-11-04 16:10:47 +00:00
Member

This error handling should happen inside the loadApp action.

This error handling should happen inside the `loadApp` action.
konrad commented 2021-11-10 19:15:27 +00:00
Author
Owner

Actually, I've simplified this so that the check does not need to happen here.

Actually, I've simplified this so that the check does not need to happen here.
👍 1
konrad marked this conversation as resolved
konrad was assigned by dpschen 2021-11-04 16:11:49 +00:00
dpschen removed their assignment 2021-11-04 16:11:55 +00:00
konrad added 2 commits 2021-11-10 18:47:11 +00:00
continuous-integration/drone/pr Build is passing Details
f0b2d7c59d
chore: move offline overlay to ready component
konrad added 1 commit 2021-11-10 18:50:32 +00:00
continuous-integration/drone/pr Build is passing Details
856a64748a
feat: add translations for offline overlay
konrad added 1 commit 2021-11-10 18:52:01 +00:00
continuous-integration/drone/pr Build is passing Details
ab0cd2af00
chore: unnest styles
konrad added 1 commit 2021-11-10 18:52:36 +00:00
continuous-integration/drone/pr Build is passing Details
f16a52e41a
chore: use @
konrad added 1 commit 2021-11-10 18:55:42 +00:00
continuous-integration/drone/pr Build is passing Details
9ff8e11171
chore: cleanup css
konrad added 1 commit 2021-11-10 18:59:21 +00:00
continuous-integration/drone/pr Build is passing Details
385f2a9a5e
chore: add explanation about naked return
konrad added 1 commit 2021-11-10 19:01:40 +00:00
continuous-integration/drone/pr Build is passing Details
f2c8cdd3be
chore: add explanation about naked return
konrad added 1 commit 2021-11-10 19:07:54 +00:00
continuous-integration/drone/pr Build is passing Details
699d5bdc88
feat: use script setup for no auth wrapper
konrad added 1 commit 2021-11-10 19:08:38 +00:00
continuous-integration/drone/pr Build is passing Details
eea2af354f
chore: start loading in created instead of mounted
konrad added 1 commit 2021-11-10 19:09:19 +00:00
continuous-integration/drone/pr Build is passing Details
27c840987e
chore: simplify css
konrad added 1 commit 2021-11-10 19:09:49 +00:00
continuous-integration/drone/pr Build is passing Details
ab4ef96fef
chore: use @ for imports
konrad added 1 commit 2021-11-10 19:11:20 +00:00
continuous-integration/drone/pr Build is passing Details
06b2632fb5
chore: simplify loader css
konrad added 1 commit 2021-11-10 19:15:09 +00:00
continuous-integration/drone/pr Build is passing Details
1cd5b41c96
chore: improve error checking for no api url
konrad added 1 commit 2021-11-10 19:16:49 +00:00
continuous-integration/drone/pr Build is passing Details
15a6e52815
chore: make if more readable
dpschen reviewed 2021-11-10 19:28:47 +00:00
src/components/misc/ready.vue Outdated
@ -0,0 +72,4 @@
return !this.ready && this.error === ''
},
...mapState({
online: ONLINE,
dpschen commented 2021-11-10 19:28:47 +00:00
Member

ONLINE is no mutation type. It's a state.

`ONLINE` is no mutation type. It's a state.
konrad commented 2021-11-10 19:53:07 +00:00
Author
Owner

Right, fixed.

Right, fixed.
konrad marked this conversation as resolved
konrad added 1 commit 2021-11-10 19:53:00 +00:00
continuous-integration/drone/pr Build is passing Details
628d102907
fix: don't use mutation type in lieu of state
dpschen reviewed 2021-11-10 20:45:01 +00:00
src/components/misc/api-config.vue Outdated
@ -76,2 +89,4 @@
async setApiUrl() {
if (this.apiUrl === '') {
// Don't try to check and set an empty url
return
dpschen commented 2021-11-10 20:45:01 +00:00
Member

That seems like it should throw an error?

That seems like it should throw an error?
konrad commented 2021-11-10 21:17:14 +00:00
Author
Owner

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.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-10 20:45:12 +00:00
src/components/misc/api-config.vue Outdated
@ -94,1 +98,3 @@
window.API_URL = urlToCheck.toString()
if (url === '') {
// If the config setter function could not figure out a url
return
dpschen commented 2021-11-10 20:45:12 +00:00
Member

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

That seems like it should throw an error? (2)
konrad marked this conversation as resolved
dpschen reviewed 2021-11-10 20:53:09 +00:00
src/helpers/checkAndSetApiUrl.ts Outdated
@ -0,0 +2,4 @@
export const ERROR_NO_API_URL = 'noApiUrlProvided'
export const checkAndSetApiUrl = (url: string, updateConfig: () => Promise<void>): Promise<string> => {
dpschen commented 2021-11-10 20:53:09 +00:00
Member

Since updateConfig is always () => dispatch('config/update'):
Shouldn't we just import he store in this file and call dispatch('config/update') directly?

Since `updateConfig` is always `() => dispatch('config/update')`: Shouldn't we just import he store in this file and call `dispatch('config/update')` directly?
konrad commented 2021-11-10 20:54:52 +00:00
Author
Owner

Will that work outside of a vue component?

Will that work outside of a vue component?
dpschen commented 2021-11-10 21:32:13 +00:00
Member

If you import the store with:

import {store} from '@/store'

// then you can

store.dispatch('config/update')

Because it's JS ™️ (trying to steal reacts claim here)

If you import the store with: ``` import {store} from '@/store' // then you can store.dispatch('config/update') ``` Because it's JS ™️ (trying to steal reacts claim here)
konrad commented 2021-11-13 16:24:43 +00:00
Author
Owner

That seems like a nice way to solve it. Done.

That seems like a nice way to solve it. Done.
👍 1
konrad marked this conversation as resolved
konrad added 1 commit 2021-11-10 21:16:29 +00:00
continuous-integration/drone/pr Build is passing Details
4ca53d77e2
feat: show errors when figuring out the url failed
konrad added 1 commit 2021-11-10 21:26:49 +00:00
continuous-integration/drone/pr Build is passing Details
d3564c3ac2
Merge branch 'main' into feature/ready-state
frederick commented 2021-11-10 21:28:45 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://926-featureready-state--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!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://926-featureready-state--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! > Beep boop, I'm a bot.
🎉 2
konrad added 2 commits 2021-11-13 16:22:27 +00:00
0f630a9c1c
Merge branch 'main' into feature/ready-state 
# Conflicts:
#	src/components/home/contentNoAuth.vue
continuous-integration/drone/pr Build is passing Details
0609b35c9b
chore: use new logo component
konrad added 1 commit 2021-11-13 16:24:26 +00:00
continuous-integration/drone/pr Build is passing Details
817372b751
chore: import the store directly to update the config
dpschen approved these changes 2021-11-13 19:39:52 +00:00
konrad merged commit 0a2d5ef820 into main 2021-11-13 19:49:03 +00:00
konrad deleted branch feature/ready-state 2021-11-13 19:49:03 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
konrad
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#926
No description provided.
Delete Branch "feature/ready-state"

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?