Persist some frontend preferences in localStorage #4559
Conversation
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
Ah yes, of course, good idea! |
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
Core Meeting Decision
|
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
carlobeltrame
had a problem deploying
to
feature-branch
GitHub Actions
Failure
| @@ -93,7 +93,7 @@ Displays a single scheduleEntry | |||
| <template v-else>{{ $tc('global.button.back') }}</template> | |||
| </v-btn> | |||
|
|
|||
| <TogglePaperSize :value="isPaperDisplaySize" @input="toggleDisplaySize" /> | |||
There was a problem hiding this comment.
Why did you remove this functionality? This prevents a layout jump when you toggle the displaySize, because it changes the responsiveLayout to be papersize before the animation takes place.
There was a problem hiding this comment.
I had to remove the previous local storage implementation, which was done in the composable useDisplaySize.js. When doing this, I overlooked the nextTick and local copy of the state in there. But I am mostly convinced it does not make any perceivable difference. If it does, can you tell me which one of these screencasts comes from dev (with nextTick) and which one comes from my local instance (without nextTick)?
Blind Test A.webm
Blind Test B.webm
However, previously the paper size display of the responsive layout was broken in this PR. I fixed this now in 82f2a9c.
There was a problem hiding this comment.
Hmm, shouldn't this be a computed and not a function in 82f2a9c?
Maybe this was required with the initial local value variant.
There was a problem hiding this comment.
I removed the computed when I switched from the composition API paradigms back to the options API way for this feature.
What advantages do you expect from using a composition API computed here over a simple function like we do for the other provided values? I did try passing down the plain value, but that isn't reactive. The Vue 3 docs explicitly recommend using a computed for this case, but it seemed inconsistent with our other provides, and I didn't want to touch all other provides. But if you insist on using computed for just this one provide, I can change it.
There was a problem hiding this comment.
I don't mind the implementation. I just hope that it won't break at random :D
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
| const [, uri, identifier] = key.match( | ||
| new RegExp(`^${LOCAL_STORAGE_PREFIX}(.*):(.*)$`) | ||
| ) | ||
| values[uri] = values[uri] || {} |
There was a problem hiding this comment.
| values[uri] = values[uri] || {} | |
| values[uri] ||= {} |
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
| @@ -3,7 +3,7 @@ import Vue from 'vue' | |||
| const LOCAL_STORAGE_PREFIX = 'preferences:' | |||
|
|
|||
| export function loadFromLocalStorage(localStorage = window.localStorage) { | |||
| let values = {} | |||
| const values = {} | |||
| let store = {} | ||
|
|
||
| return { | ||
| getStore: () => store, |
There was a problem hiding this comment.
minor:
It is a stupid api design from the w3c/javascript spec that the store is exposed.
But if you implement the clear like it is now,
the store returned by getStore is not cleared by the clear function.
I don't know if it is like this in the specification or if it is intended.
There was a problem hiding this comment.
getStore isn't in the ecmascript spec I think, I added this for debugging. It isn't needed anymore, I will remove it.
| it('loads saved picasso edit mode true', async () => { | ||
| // given | ||
| const state = loadFromLocalStorage({ | ||
| 'preferences:/camps/1a2b3c4d:picassoEditMode': 'true', |
There was a problem hiding this comment.
minor:
the prefix could be extracted into a constant to simplify changing it
the camp iri could also be a constant, that you see from afar if two different constants point to the same camp or not.
There was a problem hiding this comment.
I kind of like the explicit-ness of the test this way, and I think a search and replace would work well if we change the prefix. But it is a matter of taste. I will replace the camp URI with a constant, but since template literals can't directly be used as keys in JS objects it will look like this:
const state = loadFromLocalStorage({
[`preferences:${CAMP_URI}:picassoEditMode`]: 'true',
})There was a problem hiding this comment.
as you like, it is a minor taste related issue
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
Persists the editMode (locked/unlocked) state of the picasso and the story overview as well as the last used print config into localStorage. All preferences are stored on a per-camp basis, and naturally being stored in localStorage, they do not carry over across different browsers, devices or even user accounts.
This PR is a proposition / prototype. We haven't formally discussed how to solve this, although both parts of the feature have been requested by multiple independent users. I'll be happy to incorporate any feedback or downsize this if needed etc.. Just wanted to get the discussion started.
Btw: Nuxt Print Preview throws a lot of console errors at the moment when running locally, probably since the move to nuxt 3. I didn't touch (nor cause) these problems.
We have to be aware that this feature makes local testing a little more difficult: It will lead to divergence between our dev environments if one person always has different preferences than another person (e.g. someone never tests print with a title page because they have their favourite print preset in their favourite testing camp). But on feature branch deployments, we always start with a clean slate.