immich
immich copied to clipboard
fix(web): Asset viewer stack thumbnails overflow on top of asset
Description
When an asset has many photos stacked, the stack thumbnails will overflow up, well into the frame. This change assigns constants to the thumbnail size calculation, and uses the larger constant to set a max-height of the stack-slideshow container. The size cuts off part of the second row, making it clear there are thumbnails to scroll to.
How Has This Been Tested?
- Create a stack with many assets (enough to create multiple rows of thumbnails)
- The stack-slideshow container is now limited in size, and will not grow over the image.
Screenshots (if appropriate)
Before:
After:
Checklist:
- [x] I have performed a self-review of my own code
- [x] I have made corresponding changes to the documentation if applicable
- [x] I have no unrelated changes in the PR.
- [x] I have confirmed that any new dependencies are strictly necessary.
- [x] I have written tests for new code (if applicable)
- [x] I have followed naming conventions/patterns in the surrounding code
- [x] All code in
src/services/uses repositories implementations for database calls, filesystem operations, etc. - [x] All code in
src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)
I added a quick update to dynamically increase the max-height of the stack-slideshow while scrolling it, to a maximum of 4x the selected thumbnail size. This feels quite a bit better, personally, when there are a lot of assets, especially on mobile.
If dynamic modifications like this aren't preferred, or if this update isn't welcome I'll remove it so this update stays at just locking the stack-slideshow height.
I think, realistically, having a single row that is horizontally scrollable is nicer. In actual usecase, people would not stack unrelated photos together
I can take a look at that. Though should I revert the last update so we can merge this, just to get the bug itself under control? Then put horizontal scroll in a separate PR?