forked from DuDigital/react-native-zoomable-view
-
Notifications
You must be signed in to change notification settings - Fork 63
feat: NonScalingOverlay translate-only marker overlay #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
thomasttvo
wants to merge
29
commits into
master
Choose a base branch
from
thomas/nonscaling-overlay
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+548
−101
Draft
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
6db57ff
feat: add NonScalingOverlay translate-only marker overlay
b65a986
chore: untrack example/ios prebuild artifact (app.json is source of t…
d075275
revert: drop example/ios/ gitignore — native edits must remain trackable
86039bd
fix: track Info.plist as source instead of app.json prebuild config
30125ff
chore: replace dead placekitten.com with picsum.photos
a760053
refactor: unify NonScalingOverlay transform — single 5-element list, …
830d98e
refactor: remove FixedSize, replaced by NonScalingOverlay translate-o…
1f29392
fix(NonScalingOverlay): anchor at top:0/left:0 to defeat Yoga centering
544d06d
chore: untrack example/ios/ — Expo Go workflow, prebuild output is ep…
7c28e54
example: drop applyContainResizeMode, size contents by aspectRatio
d4b9734
example(metro): dedupe peers via resolveRequest, drop semver pin
b8a6df6
refactor: drop inverseZoom/inverseZoomStyle from context — dead surfa…
e76d21f
chore(test): add jest + RTL + reanimated mock infra
d41f10d
refactor(NonScalingOverlay): extract computeOverlayTransform + unit t…
0ae8f80
test(NonScalingOverlay): component tests for EC-NSO-1, 2, 7, 8, 9
a22d18e
test(ReactNativeZoomableView): renderOverlay integration tests for EC…
011a973
refactor(helper): extract calcShiftDelta from _calcOffsetShiftSinceLa…
404e3ba
refactor(helper): extract applyPinchSensitivity for pinch resistance …
cfab3c8
refactor(helper): extract clampZoom for pinch min/max clamp
3be2f77
refactor(helper): extract shouldSkipShift predicate from _handleShifting
cced0e0
chore(test): tag canvas gesture with withTestId for jest-utils
a35ec51
test(helper): unit tests for pure-helper SPEC items
bf04142
test(tree+StaticPin): tree topology + StaticPin styling (SPEC-086, 08…
6311430
test(RNZV): props + imperativeHandle + callbacks (SPEC-008/011-018/03…
4af5533
test(RNZV): static-pin + feedback + useLatestWorklet (SPEC-042-051, 0…
2fe4687
test(gestures): tap classification — single/double/long-press (SPEC-0…
9c611f6
test(gestures): pan callbacks + onPanResponderMoveWorklet intercept (…
563d90a
test(gestures): pinch + shift + multi-finger (SPEC-014, 058, 064, 088…
3b84a9b
revert: move test suite to dedicated PR
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The example app ships debug-only visualizations the author themselves flagged for removal:
overlayDebugBox(red 18%-opacity rect with magenta border),overlayDebugHud(yellow text overlay with raw transform math), and aDBG contentSize:Text line below the zoomable view — all with comments saying "Example-only — remove for production". Separately, the inline comment on the contents View (around lines 320-336) describesmaxWidth/maxHeight: '100%'andalignSelf: 'center'styles that don't exist anywhere —styles.contentsonly setsalignSelf: 'stretch', and the style.ts comment is the accurate one. Either delete the debug surfaces and fix the comment before merge, or gate them behind a__DEV__toggle.Extended reasoning...
Two cleanup items in
example/App.tsx, both example-only1. Debug surfaces marked "remove for production" still ship unconditionally
Three debug-only render outputs are committed:
example/App.tsx:288-289—<View style={styles.overlayDebugBox} />, immediately preceded by the comment/* DEBUG: visualize the overlay's bounding box. ... Example-only — remove for production. */. The style itself (example/style.ts:60-72) is a magenta-bordered, 18%-opacity red rect filling the overlay; its own comment readsExample-only debug visualization for NonScalingOverlay's bounding box. ... Not exported by the library.example/App.tsx:290-306— yellowoverlayDebugHudText pinned to the overlay's top-left, displaying raw transform math:NSOL wW≈… wH≈… cW=… cH=… tXjs=… tYjs=…. Style atexample/style.ts:73-85.example/App.tsx:362-366—<Text>DBG contentSize: …×… box: …×…</Text>rendered below the zoomable view (outside theshowMarkersgate — it always renders).The overlay debug surfaces are gated only by
showMarkers, which is a user-facing toggle defaulting totrue. The DBG Text below has no gate at all.2. App.tsx contents-View comment contradicts the actual styles
example/App.tsx:320-336(the comment on the contents<View>) describes the layout strategy as:But
example/style.ts:28-41(styles.contents) only setsalignSelf: 'stretch'. There is nomaxWidth, nomaxHeight, andalignSelfisstretch, notcenter. The only inline override applied at use time is{ aspectRatio: sourceAspect }. The neighboringstyle.tscomment (alignSelf: 'stretch'anchors cross-axis,aspectRatioderives main-axis, parent'sjustifyContent: 'center'centers it vertically) is the accurate description — so the two comments actively contradict each other.Step-by-step proof
Take
example/App.tsxat HEAD on this branch, render in a simulator with default props (showMarkers = true, the initial state on line 196):renderOverlayfires the truthy branch (line 273-330).<View style={styles.overlayDebugBox} />— a magenta-bordered, red 18%-opacity rectangle fills the entire overlay region.<Text style={styles.overlayDebugHud}>NSOL wW≈… cH=…</Text>paints a yellow band of debug math across the top of the image.<Text>DBG contentSize: …×… box: …×…</Text>renders unconditionally — flippingshowMarkersoff does not hide it.For the comment mismatch:
grep -n 'maxWidth\|maxHeight\|alignSelf' example/style.tsreturns exactly one match —alignSelf: 'stretch'insidestyles.contents. The stringsmaxWidth,maxHeight, andalignSelf: 'center'cited by the App.tsx comment do not appear anywhere in the file.Why this is "nit" severity
This is the example app, not library source — published consumers are unaffected. The library exports
NonScalingOverlayand therenderOverlayprop; the debug rectangles, HUD, and DBG line live in the consumer-side demo and never reach the npm artifact. But the example app is the library's primary public demo ("Example app: pinch / pan with defaultStaticPin" appears in this PR's own test plan), and the author's own "Example-only — remove for production" annotation is the strongest possible evidence the items are unintentional carryover from development.Fix
Either:
if (__DEV__ && debug)(or a dedicated state toggle distinct fromshowMarkers) so the default demo experience is clean.Either way, update the App.tsx contents-View comment (lines 320-336) to match what's actually applied — drop the
maxWidth/maxHeight/alignSelf: centerdescription and reference the samealignSelf: stretch + aspectRatioexplanation that the style.ts comment already gives.