feat: add native overlay entering animations#26
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds marker and cluster entering-animation types, normalizes them into native descriptors, forwards them through map props and adapters, and executes them in iOS and Android overlay pipelines. The example app and documentation are updated to show the new behavior. ChangesOverlay Entering Animations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt`:
- Line 245: The enter animation in MapOverlayController’s marker handling is not
being tracked, so stale animators can keep writing alpha on retained markers and
cause flicker. Update the marker enter-animation flow around the alpha reset and
the animator setup in the affected marker refresh paths so each created animator
is stored and canceled or replaced when a marker is retained. Use the existing
marker lifecycle methods in MapOverlayController to ensure old animations cannot
mutate the same marker after a refresh.
- Around line 267-319: The Android entering-animation path in animateEntering
currently only updates marker.alpha, so
ResolvedOverlayEnteringAnimationKind.FADE_SCALE is never actually applied and
behaves like a plain fade. Update the shared animator logic in animateEntering
to branch on the resolved animation kind from OverlayEnteringAnimationResolver
and apply the scale transform alongside alpha for FADE_SCALE, while keeping
revealAnimatedMarkers responsible for restoring the final marker state after
animation end/cancel.
In `@package/ios/GoogleMapOverlayController.swift`:
- Around line 188-191: The Google iOS overlay path in GoogleMapOverlayController
is resolving `.fadeScale`, but the downstream Google marker helper only applies
opacity, so the scale part is never used. Update the Google-backed animation
flow around `enteringAnimation(for:)`,
`OverlayEnteringAnimationResolver.prepareGoogleMarker`, and
`OverlayEnteringAnimationResolver.animateGoogleMarker` to also apply a scale
transform when the animation is `fadeScale`, while keeping `.fade` behavior
unchanged.
- Around line 211-223: The enteringAnimation path in GoogleMapOverlayController
should explicitly apply the native Google Maps appear animation when the
resolved animation is .system, since the Google helper returns early and
GMSMarker.appearAnimation otherwise stays .none. Update the marker and cluster
insertion flow around enteringAnimation/OverlayEnteringAnimationResolver so
that, before assigning marker.map = mapView, the marker’s native appear
animation is set appropriately for the .system case.
In `@package/ios/NitroPinAnnotationView.swift`:
- Around line 21-23: The reusable pin view is carrying over mutated visual state
from interrupted custom insert animations, so reset the view before applying a
new marker in NitroPinAnnotationView.configure(for:) (especially alpha and
transform, alongside any other transient animation state) and then reapply the
marker-specific animatesWhenAdded logic. Make the reset happen every time the
annotation view is configured/dequeued so a .system or .none marker cannot
inherit a transparent or scaled state from a prior fade/fade-scale insert.
In `@package/ios/OverlayEnteringAnimation.swift`:
- Around line 82-99: The Google marker path in prepareGoogleMarker and
animateGoogleMarker only changes marker.opacity, so .fadeScale is being treated
like a plain fade. Update the Google-backed animation branch in
OverlayEnteringAnimation to either apply a scale animation to an iconView/layer
when animation.kind is .fadeScale, or map .fadeScale to .fade before this path
so the behavior is consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66750d5e-ca57-4500-8aaa-9ec58d1b9da9
📒 Files selected for processing (37)
CONTEXT.mdREADME.mddocs/adr/0003-native-overlay-entering-animations.mddocs/architecture.mddocs/roadmap.mdexample/App.tsxexample/examples/landmarks.tspackage/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/HybridMapView.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MapProviderAdapter.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerClusterEngine.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerViewportFilter.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/OverlayEnteringAnimation.ktpackage/ios/AppleMapProviderAdapter.swiftpackage/ios/GoogleMapOverlayController.swiftpackage/ios/GoogleMapProviderAdapter.swiftpackage/ios/HybridMapView.swiftpackage/ios/HybridMapViewDelegate.swiftpackage/ios/MapClusterAnnotation.swiftpackage/ios/MapMarkerAnnotation.swiftpackage/ios/MapOverlayController.swiftpackage/ios/MapProviderAdapter.swiftpackage/ios/MarkerClusterEngine.swiftpackage/ios/MarkerViewportFilter.swiftpackage/ios/NitroPinAnnotationView.swiftpackage/ios/OverlayEnteringAnimation.swiftpackage/src/components/MapView.tsxpackage/src/hooks/useCollectedOverlays.tspackage/src/index.tspackage/src/native/specs/MapView.nitro.tspackage/src/native/specs/overlays.tspackage/src/types/index.tspackage/src/types/map.tspackage/src/types/overlays.tspackage/src/utils/enteringAnimation.tspackage/type-tests/provider-props.ts
248743a to
4f3a51d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@example/App.tsx`:
- Around line 337-361: ScalePressable currently does not forward accessibility
props, so the selected state in this animation option list is not exposed to
screen readers. Update the ScalePressable component to accept and pass through
accessibilityRole, accessibilityState, and accessibilityLabel to
AnimatedPressable, then use those props in the animation selector where
onSelectAnimation and ANIMATION_OPTIONS are rendered so the active option is
announced correctly.
- Around line 584-591: The demo initializes the animation picker to the wrong
default, so the UI starts on Scale instead of the documented API default. Update
the initial value used by App.tsx’s animation state in the App component so
animationOptionIndex points to system in ANIMATION_OPTIONS, keeping the picker
and displayed default in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Summary
Validation
Runtime evidence