Add Button to expand recents. Simplify localStorage.#592
Conversation
subdavis
force-pushed
the
desktop/recents-updates
branch
from
291f557 to
d7057d0
Compare
|
Forgot to request. |
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
Works perfectly fine. I have that one comment but I don't feel strongly about it. The end result is clear because of the comments and I imagine any future issue would begin with following the data path and noting that the Image lists get removed.
| /** | ||
| * Erase image lists from meta object stored in recents. | ||
| * Saves space and serialization cost since these parts of | ||
| * the recents object aren't used in this way. | ||
| */ | ||
| imageData: [], | ||
| originalImageFiles: [], | ||
| transcodedImageFiles: [], | ||
| }); |
There was a problem hiding this comment.
I know it's a pain but I'm wondering if we should really be using a different type here given this comment and the one above about the localCachedMeta being different from JsonMeta?
There was a problem hiding this comment.
Valid. I originally did this. There's a couple of ways to hack "subtraction" to extends an interface by removing fields in typescript. I ended up not liking the results and just went with the comment.
This actually goes against my philosophy about never telling lies to the type system, but it just felt so weird to redeclare a core interface here.
What approach would you prefer?
There was a problem hiding this comment.
I looked into the subtraction stuff quickly when I was doing my review and I had the same reaction.
I say a new type that has the overlapping data expected for the frontend cached dataset to work. It clearly separates them as localCacheMeta is derived from JsonMeta and not the same thing.
It also gives a place to store future optional data that is relevant to the front-end display but not required to be stored in JsonMeta. Like we may want to edit the load of recents process to indicate to the user that the source file/folder is missing (I don't know if we want to test for sourcedata for each item in recents on each app load, just an example). That could be added as an optional localCacheMeta property easily showing the user that some of their recents have been moved or deleted outside of DIVE.
2 participants
At one point earlier today I managed to write 400 lines of code including a full schema migration to support a new field called
lastUpdated. I discarded all those changes and re-wrote it to be as simple as possible.fixes #591