Repository navigation
Conversation
0583fbd to
45a5d41
Compare
f7987ac to
5066203
Compare
|
Deploying preview environment to https://pr-24904.preview.internal.immich.build/ |
387dd5e to
457076f
Compare
5066203 to
e207760
Compare
d6d7cbc to
d138957
Compare
d138957 to
d2f0364
Compare
21e540d to
e312b45
Compare
7139757 to
70953a5
Compare
e312b45 to
7b55c0e
Compare
70953a5 to
49791dd
Compare
7b55c0e to
3d573e1
Compare
perf - replace broadcast channel with direct postMessage
preload thumbs and previews
3d573e1 to
d4901c8
Compare
immich-push-o-matic
Bot
added
documentation
49791dd to
f58dcb2
Compare
f58dcb2 to
a705be8
Compare
3e17d1a to
518dd89
Compare
refactor: replace onPreviousAsset/onNextAsset with onSwipe
518dd89 to
270b5ce
Compare
midzelis
removed
documentation
| const loader = new AdaptiveImageLoader( | ||
| asset, | ||
| undefined, | ||
| { | ||
| currentZoomFn: () => 1, | ||
| onQualityUpgrade: (url) => setPreviewUrl(direction, url), | ||
| }, | ||
| loadImage, | ||
| ); |
There was a problem hiding this comment.
I literally have no idea what any of these parameters actually are without opening an IDE. I would much prefer if the class takes an object instead. (I'm aware that's a change in the previous PR, just noticed it here right now)
| // When moving forward: old next becomes current, shift preview URLs | ||
| const oldNextUrl = nextPreviewUrl; | ||
| destroyPreviousPreloader(); | ||
| } | ||
|
|
||
| if (shouldDestroyNext) { | ||
| previousPreviewUrl = oldNextUrl; |
There was a problem hiding this comment.
You're saying "old becomes current", which I agree with. However, you then set previousPreviewUrl = nextPreviewUrl. That's not right, is it?
| } else { | ||
| // Non-adjacent navigation (e.g., slideshow random) | ||
| previousPreloader = startPreloader(newCursor.previousAsset); | ||
| nextPreloader = startPreloader(newCursor.nextAsset); | ||
| return 'skip'; | ||
| } |
There was a problem hiding this comment.
This doesn't need to be in an else
| imageClass="object-contain" | ||
| slideshowState={$slideshowState} | ||
| slideshowLook={$slideshowLook} | ||
| /> |
There was a problem hiding this comment.
More comments for the adaptive-image PR. You're only passing in global state anyways; do you even need these props?
| onSwipeEnd?: (offsetX: number) => void; | ||
| onSwipeMove?: (offsetX: number) => void; |
There was a problem hiding this comment.
What is the use case for these props?
| clientWidth?: number; | ||
| clientHeight?: number; |
There was a problem hiding this comment.
What do we need those for? Especially when we already bind to element
| clientWidth?: number; | ||
| clientHeight?: number; | ||
| reset?: () => void; | ||
| children: Snippet; |
There was a problem hiding this comment.
What do we need children for when we already bind to element? Really, a lot of these props feel either redundant or overly generic to me, could you please revisit them?
There was a problem hiding this comment.
Why are we even building our own swipe component? Surely there are already libraries for this out there
| const exifDimensions = $derived( | ||
| asset?.exifInfo?.exifImageHeight && asset?.exifInfo.exifImageHeight | ||
| ? (getDimensions(asset.exifInfo) as { width: number; height: number }) | ||
| : null, | ||
| ); |
There was a problem hiding this comment.
You should use the height and width on the asset instead, since that is also robust against edits (for whenever we get video edits)
| video:focus { | ||
| outline: none; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
Why can't we just put this on the component?
9de546e to
502eeba
Compare
|
This PR has a lot of issues, but the biggest issue is that it is way too big. Please submit pull requests that are of a reasonable size and scope. |
4 participants
feat: web - swipe feedback - show image while swiping/dragging left/right