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

fix: setting background to state mutation violation #858

Merged
dpschen merged 3 commits from fix/list-background-vuex into vue3 2021-10-16 15:51:28 +00:00
Conversation 0 Commits 3 Files Changed 4 +39 -32
4 changed files with 39 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/components/home/contentAuth.vue
View File

@ -86,7 +86,7 @@ export default {
this.$route.name === 'user.settings' ||
this.$route.name === 'namespaces.index'
) {
this.$store.commit(CURRENT_LIST, null)
return this.$store.dispatch(CURRENT_LIST, null)
dpschen marked this conversation as resolved Outdated
dpschen commented 2021-10-16 11:49:42 +00:00
Outdated
Review
Copy Link

return the result => make async

return the result => make async
konrad commented 2021-10-16 11:56:59 +00:00
Outdated
Review
Copy Link

You mean the whole action?

You mean the whole action?
dpschen commented 2021-10-16 13:09:36 +00:00
Outdated
Review
Copy Link

Yes. Same reason as in the other answer below.

Yes. Same reason as in the other answer below.
dpschen commented 2021-10-16 15:44:53 +00:00
Outdated
Review
Copy Link

Done

Done
}
},
renewTokenOnFocus() {

66
src/store/index.js
View File

@ -1,5 +1,6 @@
import { createStore } from 'vuex'
import {createStore} from 'vuex'
import {
BACKGROUND,
CURRENT_LIST,
ERROR_MESSAGE,
HAS_TASKS,
@ -62,10 +63,33 @@ export const store = createStore({
state.online = !!import.meta.env.VITE_IS_ONLINE || online
},
[CURRENT_LIST](state, currentList) {
state.currentList = currentList
},
[HAS_TASKS](state, hasTasks) {
state.hasTasks = hasTasks
},
[MENU_ACTIVE](state, menuActive) {
state.menuActive = menuActive
},
toggleMenu(state) {
state.menuActive = !state.menuActive
},
[KEYBOARD_SHORTCUTS_ACTIVE](state, active) {
state.keyboardShortcutsActive = active
},
[QUICK_ACTIONS_ACTIVE](state, active) {
state.quickActionsActive = active
},
[BACKGROUND](state, background) {
state.background = background
},
},
actions: {
async [CURRENT_LIST]({state, commit}, currentList) {
konrad marked this conversation as resolved Outdated
dpschen commented 2021-10-16 11:50:42 +00:00
Outdated
Review
Copy Link

With [CURRENT_LIST]({state}, currentList) we could keep the diff smaller. But not sure if that makes sense

With `[CURRENT_LIST]({state}, currentList)` we could keep the diff smaller. But not sure if that makes sense
konrad commented 2021-10-16 11:58:49 +00:00
Outdated
Review
Copy Link

I think it does make sense, changed it.

I think it does make sense, changed it.
if (currentList === null) {
state.currentList = {}
state.background = null
commit(CURRENT_LIST, {})
commit(BACKGROUND, null)
return
}
@ -97,21 +121,18 @@ export const store = createStore({
)
) {
if (currentList.backgroundInformation) {
const listService = new ListService()
listService.background(currentList)
.then(b => {
state.background = b
})
.catch(e => {
console.error('Error getting background image for list', currentList.id, e)
})
} else {
state.background = null
try {
const listService = new ListService()
dpschen marked this conversation as resolved Outdated
dpschen commented 2021-10-16 11:51:11 +00:00
Outdated
Review
Copy Link

Not that this is an action maybe we can already use await here.

Not that this is an action maybe we can already use await here.
dpschen commented 2021-10-16 15:45:06 +00:00
Outdated
Review
Copy Link

Done

Done
const background = await listService.background(currentList)
commit(BACKGROUND, background)
} catch(e) {
console.error('Error getting background image for list', currentList.id, e)
}
}
}
if (typeof currentList.backgroundInformation === 'undefined' || currentList.backgroundInformation === null) {
state.background = null
commit(BACKGROUND, null)
}
// Server updates don't return the right. Therefore the right is reset after updating the list which is
@ -120,22 +141,7 @@ export const store = createStore({
if (typeof state.currentList.maxRight !== 'undefined' && (typeof currentList.maxRight === 'undefined' || currentList.maxRight === null)) {
currentList.maxRight = state.currentList.maxRight
}
state.currentList = currentList
},
[HAS_TASKS](state, hasTasks) {
state.hasTasks = hasTasks
},
[MENU_ACTIVE](state, menuActive) {
state.menuActive = menuActive
},
toggleMenu(state) {
state.menuActive = !state.menuActive
},
[KEYBOARD_SHORTCUTS_ACTIVE](state, active) {
state.keyboardShortcutsActive = active
},
[QUICK_ACTIONS_ACTIVE](state, active) {
state.quickActionsActive = active
commit(CURRENT_LIST, currentList)
},
},
})

1
src/store/mutation-types.js
View File

@ -7,6 +7,7 @@ export const HAS_TASKS = 'hasTasks'
export const MENU_ACTIVE = 'menuActive'
export const KEYBOARD_SHORTCUTS_ACTIVE = 'keyboardShortcutsActive'
export const QUICK_ACTIONS_ACTIVE = 'quickActionsActive'
export const BACKGROUND = 'background'
export const CONFIG = 'config'
export const AUTH = 'auth'

2
src/views/list/ShowList.vue
View File

@ -141,7 +141,7 @@ export default {
const list = new ListModel(listData)
this.listService.get(list)
.then(r => {
this.$store.commit(CURRENT_LIST, r)
this.$store.dispatch(CURRENT_LIST, r)
this.setTitle(this.getListTitle(r))
dpschen commented 2021-10-16 11:51:45 +00:00
Review
Copy Link

Really not sure here: should the setTitle await for the action to finish?

Really not sure here: should the setTitle await for the action to finish?
konrad commented 2021-10-16 12:01:48 +00:00
Review
Copy Link

I think not. Basically, all that the action does that is async is loading the background. The rest are mutations which happen almost immedieatly (from the user's perspective anyway). I think if the list itself already loaded it is rather confusing if it is waiting for no other reason than to load the list background. Especially since I plan to use BlurHash in the future for list backgrounds where it would not wait until the full background is loaded.

I think not. Basically, all that the action does that is async is loading the background. The rest are mutations which happen almost immedieatly (from the user's perspective anyway). I think if the list itself already loaded it is rather confusing if it is waiting for no other reason than to load the list background. Especially since I plan to use [BlurHash](https://blurha.sh/) in the future for list backgrounds where it would not wait until the full background is loaded.
dpschen commented 2021-10-16 13:08:22 +00:00
Review
Copy Link

Not sure if I got that or if I'm missing something: if you await the dispatch just the setTitle would need to wait. And that's something you could change by reversing the order. The loadList function then has to be async for sure. But since it's not awaited anywhere no code should wait.
The only thing that changes is that the loadList returns a promise that says when it's finished. We don't need that right now, but if we do need it in the future the function is already prepared.

That's nice! BlurHash is really cool :)
Do you know sqip.
No idea how that compares, but I also always wanted to try that.

What I never got with BlurHash is why you would need to implement a new way of hashing images when you can just create a really small image as e.g. webp, inline that and blur it via CSS. Obviously that doesn't work as simple native so there you would need to adapt the implementation, something that BlurHash already does for you.

Not sure if I got that or if I'm missing something: if you await the dispatch just the `setTitle` would need to wait. And that's something you could change by reversing the order. The loadList function then has to be async for sure. But since it's not awaited anywhere no code should wait. The only thing that changes is that the loadList returns a promise that says when it's finished. We don't need that right now, but if we do need it in the future the function is already prepared. ---- That's nice! BlurHash is really cool :) Do you know [sqip](https://www.freecodecamp.org/news/using-svg-as-placeholders-more-image-loading-techniques-bed1b810ab2c/). No idea how that compares, but I also always wanted to try that. What I never got with BlurHash is why you would need to implement a new way of hashing images when you can just create a really small image as e.g. webp, inline that and blur it via CSS. Obviously that doesn't work as simple native so there you would need to adapt the implementation, something that BlurHash already does for you.
dpschen commented 2021-10-16 13:18:14 +00:00
Review
Copy Link

Ahh I just found this note:

blurhash - Really nice, compact placeholder images.
Requires non-native client-side decoding which makes it awkward and slow for browser usage.
Encoding speed is pretty slow (on par with sqip).
Under the hood, the webp format performs a similar set of transforms as the one used by blurhash.

See here: https://github.com/transitive-bullshit/lqip-modern#related

Ahh I just found this note: > blurhash - Really nice, compact placeholder images. > Requires non-native client-side decoding which makes it awkward and slow for browser usage. > Encoding speed is pretty slow (on par with sqip). > Under the hood, the webp format performs a similar set of transforms as the one used by blurhash. See here: https://github.com/transitive-bullshit/lqip-modern#related
konrad commented 2021-10-16 18:35:34 +00:00
Review
Copy Link

sqip looks great! Didn't know it before. I guess I'll do some experiments about the performance of both when it's time to implement it.

sqip looks great! Didn't know it before. I guess I'll do some experiments about the performance of both when it's time to implement it.
🚀 1
})
.catch(e => {