#2391 - Multiple images in the viewer #2419
Conversation
…ltiple-images-in-the-viewer
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
sebin/task/#2391-multiple-images-in-the-viewer
|
| Run status |
|
| Run duration | 09m 00s |
| Commit |
|
| Committer | Sebin Song |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
111
|
| View all changes introduced in this branch ↗︎ | |
| @@ -5,7 +5,7 @@ | |||
| @mouseup='mouseUpHandler' | |||
| ) | |||
| img.c-preview-image(ref='previewImg' | |||
| :class='{ "is-movable": isImageMovable }' | |||
| :class='{ "is-movable": isImageMovable, "is-hidden": !ephemeral.imgInitDone }' | |||
There was a problem hiding this comment.
Discovered a bug where the image kind of flickers when it finishes the load/initialization steps(happens because image dimensions changes from it's natural size to the initial zoom value). So made a fix here.
There was a problem hiding this comment.
Awesome! I noticed this behavior too but thought it was Android's fault.
| ownerID: this.ownerID, | ||
| imgUrl: entry.url || this.objectURLList[index] || '', | ||
| createdAt: this.createdAt || new Date(), | ||
| id: randomHexString(12) |
There was a problem hiding this comment.
Instead of a random ID, wouldn't it be better to use a deterministic ID in this case? For example, I think the URLs would be both deterministic and unique (or it could be combined with the message ID).
There was a problem hiding this comment.
Valid suggestion. But I think it's safe to use the random string here because the id here is merely used for differentiating multiple images rendered in the DOM.
There was a problem hiding this comment.
Yeah, I think it's safe also as used here, but I'm partial to deterministic IDs because they're always safe and can also be used, e.g., in testing.
| this.ephemeral.currentIndex = this.initialIndex | ||
| } | ||
|
|
||
| this.touchMatchMedia = window.matchMedia('(hover: none) and (pointer: coarse)') |
There was a problem hiding this comment.
I think this is the same issue as on the other PR with not having this in data ()...
| ) | ||
| } | ||
| }) | ||
| const initialIndex = allImageAttachments.findIndex(attachment => attachment.imgUrl === objectURL) || 0 |
There was a problem hiding this comment.
findIndex returns -1 when an image isn't found, not falsy. This || 0 wouldn't get triggered on -1.
There was a problem hiding this comment.
Good catch. Fixed.
|
|
||
| const args = { changeFactor: changeFactorToUse, center } | ||
| if (changeFactorTooLarge) { return } |
There was a problem hiding this comment.
For some reason Claude thinks line 128 below is an issue and says:
Failure to Update State on Large Change Factor
I'm not sure what to make of this comment. Feel free to ignore if nothing.
There was a problem hiding this comment.
Yes, this line was added intentionally (to remove a sudden jump in zoom value when pinch action was performed quickly and stopped) and added a NOTE: comment for this as well in line L102
…ltiple-images-in-the-viewer
closes #2391
[Desktop]
It also allows navigation via arrows in the keyboard too.
[Mobile]