Conversation
…ition rendering
Internal restructure of <ScriptGoogleMapsOverlayView> with one
behaviour-visible side effect (DOM structure change).
Highlights:
- Reactive position. Position updates from `draw()` write to a
`overlayPosition` shallowRef that the template binds via `:style` on
the anchor element. Vue's patcher updates the moved DOM node, so
imperative `el.style.left/top/visibility/...` writes are gone. The
`data-state` attribute is also a reactive template binding now,
removing the imperative `setDataState` helper that previously
propagated state to both the wrapper and its first child.
- Anchor + content split. The overlay renders a two-element structure:
the anchor div (positioned, moved into a Google Maps pane) wraps the
content div (carries `data-state` + forwarded `class`/attrs + the
slot). This is the DOM-level breaking change; consumers that target
`[data-state]` on the slot's first child should move that class to
the OverlayView component itself.
- Class factory. The `CustomOverlay extends mapsApi.OverlayView`
declaration is lifted out of `useGoogleMapsResource`'s `create()`
callback into a `makeOverlayClass(mapsApi, map)` factory at script
setup top level, so the `create()` callback is just wiring.
- Playground demo update. `overlay-animated.vue` moves the
`.overlay-popup` class from a slot child div onto the
`<ScriptGoogleMapsOverlayView>` itself and uses `:deep()` in scoped
styles to reach the now-internal content element.
Tests:
Five new mount-based tests in google-maps-overlay-view.nuxt.test.ts
covering the anchor → content → slot DOM hierarchy, the projected
pixel position landing on the anchor's inline style, the reactive
visibility transition when the controlled `open` prop flips to false,
the anchor staying in the DOM during the close path (so CSS
animations targeting `[data-state="closed"]` can run), and data-state
exposure on the content div for CSS targeting.
All v1 protected behaviours remain green:
- skip-if-equal setCenter watcher
- map.setOptions exclusion of zoom/center
- defineModel('open', { default: undefined })
- data-state="open"/"closed" toggling
- InfoWindow group close
100/100 google-maps-* tests pass; lint and typecheck clean.
commit: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis change refactors the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`:
- Around line 237-244: The pan logic triggers even when the overlay is closed or
unpositioned; change the block using panOnOpen so it first checks the overlay's
open state and that overlayAnchor.value is positioned before scheduling
panMapToFitOverlay. Specifically, update the conditional around panOnOpen to
also verify the component's open flag (open) and that overlayAnchor.value exists
and has a valid position (e.g., overlayAnchor.value.getPosition() or similar)
before calling requestAnimationFrame and panMapToFitOverlay; keep the existing
padding calculation and only call panMapToFitOverlay when those guards pass.
🪄 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: 99b015f1-0ebf-474c-a9bb-5796c8f56462
📒 Files selected for processing (4)
docs/content/docs/4.migration-guide/1.v0-to-v1.mdpackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vueplayground/pages/third-parties/google-maps/overlay-animated.vuetest/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts
| if (panOnOpen) { | ||
| // Wait for draw() to position the element, then pan | ||
| const padding = typeof panOnOpen === 'number' ? panOnOpen : 40 | ||
| requestAnimationFrame(() => { | ||
| if (overlayAnchor.value) | ||
| panMapToFitOverlay(overlayAnchor.value, map, padding) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Guard pan-on-open so closed/unpositioned overlays do not pan the map.
On Line 237, panMapToFitOverlay is scheduled whenever panOnOpen is enabled, even if open is false or draw() cannot resolve a position. That can cause unexpected map panning on initial mount/remount while the overlay is closed.
🔧 Suggested fix
- if (panOnOpen) {
+ if (panOnOpen && open.value !== false) {
// Wait for draw() to position the element, then pan
const padding = typeof panOnOpen === 'number' ? panOnOpen : 40
requestAnimationFrame(() => {
- if (overlayAnchor.value)
+ if (overlayAnchor.value && overlayPosition.value)
panMapToFitOverlay(overlayAnchor.value, map, padding)
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`
around lines 237 - 244, The pan logic triggers even when the overlay is closed
or unpositioned; change the block using panOnOpen so it first checks the
overlay's open state and that overlayAnchor.value is positioned before
scheduling panMapToFitOverlay. Specifically, update the conditional around
panOnOpen to also verify the component's open flag (open) and that
overlayAnchor.value exists and has a valid position (e.g.,
overlayAnchor.value.getPosition() or similar) before calling
requestAnimationFrame and panMapToFitOverlay; keep the existing padding
calculation and only call panMapToFitOverlay when those guards pass.
There was a problem hiding this comment.
Good catch — verified this is pre-existing on main: the if (panOnOpen) { requestAnimationFrame(...) } block had no open/position guard before this PR either, the refactor just moved it into the new factory. Keeping PR E focused on the class extraction + reactive position rendering and tracking the pan-on-open guard as a separate follow-up so the fix lands with its own regression test.
There was a problem hiding this comment.
@harlan-zw, thanks for verifying the pre-existing behaviour — makes sense to keep the fix out of scope here and ship it alongside a dedicated regression test. Would you like me to open a GitHub issue to track the pan-on-open guard fix (adding the open.value !== false and overlayPosition.value guards with a regression test)?
1 participant
🔗 Linked issue
Related to #689 (PR E in the umbrella split-up plan; final piece, depends on #696).
❓ Type of change
📚 Description
Internal restructure of
<ScriptGoogleMapsOverlayView>that replaces imperative DOM manipulation with reactive Vue bindings, with one behaviour-visible side effect (DOM structure change).The motivation: the previous implementation wrote
el.style.left/top/visibility/transformandel.dataset.stateimperatively from insideOverlayView.draw(), then propagateddata-stateto the slot's first child manually because Vue couldn't patch elements that had been reparented out of the component tree. The reactive pattern in this PR demonstrates that Vue's:styleand:data-statebindings actually do patch moved DOM nodes correctly, so we can drop the imperative writes entirely and rely on the framework.The slot now sits inside an additional
overlay-anchordiv.The DOM hierarchy is now
anchor div → content div → slot(previouslycontent div → slot). The anchor handles positioning and gets reparented into a Google Maps pane ononAdd(); the content div carriesdata-stateplus any forwardedclass/attrs.Selectors targeting
[data-state="open"]on the slot's first child will not match anymore, becausedata-statelives on the internal content div now and is no longer propagated into the slot.📝 Migration
Move the class onto the OverlayView component itself; it gets forwarded onto the content div via
v-bind=\"\$attrs\". Use:deep()in scoped styles to reach across the component boundary:```diff
- <ScriptGoogleMapsOverlayView class="overlay-popup" v-model:open="open">
- ...
-
<style scoped> -.overlay-popup[data-state=\"open\"] { animation: ... } +:deep(.overlay-popup[data-state=\"open\"]) { animation: ... } </style>```
🛠 Implementation notes
overlayPositionshallowRef is written bydraw(). The template binds:style=\"overlayStyle\"on the anchor element, whereoverlayStyleis a computed that derives left/top/transform/visibility from the position + offset/anchor/zIndex props. Vue's patcher updates the moved DOM node directly.data-state.dataStateis a computed bound on the content div. No imperativedataset.statewrites, no need to propagate to slot children.class CustomOverlay extends mapsApi.OverlayViewis wrapped in amakeOverlayClass(mapsApi, map)factory at script-setup top level. The class can't be a true module-top-level declaration (it depends on the runtime base class from the loaded Maps API), but the factory keepscreate()focused on instantiation and listener wiring.display: nonewrapper in the component template, thenonAdd()callspanes[pane].appendChild(anchorEl)to move it into the Google Maps pane. Vue's reactivity continues to patch the moved element because the vdom reference doesn't depend on the parent.:stylebinding (instead of imperative writes) keeps the element in the DOM afteropenflips false, so CSS animations targeting[data-state=\"closed\"]can actually run before the element is hidden. (Not yet wired up to wait fortransitionend, but the structural prerequisite is now in place.)🛡 Protected behaviours (regression-tested)
All v1 protected behaviours remain green:
setCenterwatcher (parent map)map.setOptionsexclusion ofzoom/centerdefineModel('open', { default: undefined })continues to treat missingv-model:openas undefined, not falsedata-state=\"open\"/\"closed\"togglingdraw()🧪 Tests
Five new mount-based tests in
test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts:anchor → content → slothierarchyopenflips tofalsedata-stateis exposed on the content div for CSS targeting100/100 google-maps-* tests pass; lint and typecheck clean.
Playground demo update
playground/pages/third-parties/google-maps/overlay-animated.vuemoves.overlay-popupfrom a slot child div onto the<ScriptGoogleMapsOverlayView>itself and uses:deep()in scoped styles to reach the now-internal content element. This is the canonical demonstration of the new class-on-component animation pattern.