Zoom and Pan UX Improvements on Mobile in History Viewer#22638
Zoom and Pan UX Improvements on Mobile in History Viewer#22638nrlcode wants to merge 2 commits intoblakeblackshear:devfrom
Conversation
| "size-full rounded-lg bg-black md:rounded-2xl", | ||
| loadedMetadata ? "" : "invisible", | ||
| "cursor-pointer", | ||
| videoClassName, |
There was a problem hiding this comment.
cursor pointer should just be joined here
There was a problem hiding this comment.
Agreed. cursor-pointer is now joined into the base class string as requested.
| videoClassName, | ||
| containerRef, | ||
| visible, | ||
| showControls = visible, |
There was a problem hiding this comment.
this is not a good pattern, I would prefer this have a default value and the places where it is needed it can just be included in the existing visible logic.
There was a problem hiding this comment.
I removed the default coupling here.
showControlsnow defaults totrue, and visibility composition is explicit:show={visible && showControls && (controls || controlsOpen)}
| videoClassName={videoClassName} | ||
| containerRef={containerRef} | ||
| visible={!(isScrubbing || isLoading)} | ||
| visible={true} |
There was a problem hiding this comment.
why are we changing this to always be visible?
There was a problem hiding this comment.
I kept visible={true} intentionally in the this update to preserve transform state while scrubbing/loading. Toggling visible to false hides the transformed wrapper (display:none), which can reflow/reclamp pan at high zoom and cause jumpy position after scrubbing. It also makes the scrubbing behavior show the full camera and not the zoomed in portion, if the user is zoomed in.
- Controls are still hidden during scrub/loading via:
showControls={!isScrubbing && !isLoading}
| const previewPlayer = ( | ||
| <PreviewPlayer | ||
| className={cn( | ||
| source ? "pointer-events-none absolute inset-0 z-20" : className, |
There was a problem hiding this comment.
why does this className change with source? This seems like an odd change. You also have !source below so this is incorrect logic. I don't think this should be changing in this way.
There was a problem hiding this comment.
I changed preview rendering to use explicit class paths:
- transformed overlay path when source exists
- simple visible/hidden path when source is missing
- This keeps class logic clear and avoids contradictory conditions, while preserving scrub preview inside the transformed layer so zoom/pan framing remains stable.
|
I think we need to be more specific and only make changes that actually pertain to this PR. Upon loading this code on desktop I see multiple problems:
These are the types of changes that are fairly complicated, and why only the minimal changes for the PR goal should be implemented |
Fair enough! I have been trying to scope the changes to only this PR, which is why I broke out the snapshot behavior from the previous one.
Implementing some of the suggestions you provided seems to have broken things further. I'll step back and re-evaluate. From a functionality standpoint, on mobile I was targeting:
If you think it should function differently I am open to suggestions. But keeping those layers in the same zoom/position/pan was difficult to lock down. |
this is not something that should be done in this PR, it is very complicated given the way these views different and have sizing constraints especially around hour transitions. This seems like something overlay complicated for AI to figure out and this PR should be considerably simpler without this |
|
So I feel like it is relevant to this PR, because otherwise scrubbing brings up the entire preview. Would we expect that it just goes back to the original view once scrubbing is done, and the user will need to zoom/pan again? I was trying to replicate the experience when viewing individual cameras live on mobile: how they move around, zoom, pan, etc... |
|
This is exactly the way it works today, adding the ability to resize the mobile timeline / preview sections is not directly related to that behavior. It is in the same area but that is separate from the new functionality. |
|
Ok I see that. I figured it was better to have the timeline itself respect the same zoom/panning that was chosen by the user when scrubbing, but I do see that the current functionality just zooms back out. The only issue I had there was that the camera was then not returning to where the user had zoomed/panned to. If I strip out a bunch of this other functionality I should be able to get back to replicating the current behavior. |
2 participants
Please read the contributing guidelines before submitting a PR.
Proposed change
This PR improves History/Recording viewing UX on mobile (portrait), especially for wide-aspect cameras.
This makes timeline review and zoomed inspection more practical on phones without requiring fullscreen-only workflows.
Type of change
Additional information
For new features
AI disclosure
AI tool(s) used (e.g., Claude, Copilot, ChatGPT, Cursor):
ChatGPT/Codex
How AI was used (e.g., code generation, code review, debugging, documentation):
Used for implementation support, refactoring cleanup, and debugging targeted History UI behavior.
Extent of AI involvement (e.g., generated entire implementation, assisted with specific functions, suggested fixes):
AI assisted with specific components and targeted fixes; final scope decisions, validation, and merge selection were directed by me.
Human oversight: Describe what manual review, testing, and validation you performed on the AI-generated portions.
I manually reviewed changed files, validated branch scope before merge selection, and ran local frontend checks (eslint, tsc --noEmit, and test/build commands used during iteration). I also created docker containers from the changes and validated that the new features exhibit the proper behavior on both mobile and desktop.
Checklist
enlocale.ruff format frigate) [N/A]