Conversation
…idening Two additive changes to <ScriptGoogleMapsOverlayView>: 1. Add `defaultOpen` prop for the uncontrolled mode. The overlay still opens on mount by default; pass `:default-open="false"` to start closed without taking control of the open state via v-model. Bound v-model:open continues to take precedence. 2. Widen the `position` prop to accept `google.maps.LatLng | LatLngLiteral` so values returned by Maps APIs (e.g. `resolveQueryToLatLng`, marker positions) can be passed straight through without manual conversion. Implementation notes: - Refactored to reactive prop destructure with inline defaults (matching the project preference); withDefaults removed - defineModel kept for the `open` model so its `default: undefined` opts out of Vue's boolean prop coercion (which would otherwise turn an unset `open` into `false` and break the uncontrolled default) - Added `normalizeLatLng` helper in `useGoogleMapsResource` that detects callable `.lat`/`.lng` accessors instead of relying on `instanceof google.maps.LatLng` (mocks in tests return plain objects) - The `props.position` watcher source is normalized so the watch identity is stable for both LatLng instances and literals - Inline default for `defaultOpen` is `true`, preserving v0 behaviour Docs: added "Controlled vs Uncontrolled Open State" and "Position Format" sections to overlay-view.md. Tests: new file `test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts` covering the uncontrolled default, defaultOpen=false, controlled explicit :open prop, and both position shapes via mountSuspended with a minimal mock OverlayView base class. Plus pure-helper tests for normalizeLatLng.
commit: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces support for both Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes DetailsComponent Changes ( Utility Addition ( Test Coverage ( Documentation ( 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
🧹 Nitpick comments (3)
docs/content/scripts/google-maps/2.api/11.overlay-view.md (1)
140-152: Code example has variable declared after use.In the
showSydneyfunction,position.valueis assigned on line 148, but thepositionref is declared on line 151. While Vue's<script setup>hoists declarations, this ordering may confuse readers following the code top-to-bottom.📝 Suggested reordering for clarity
<script setup lang="ts"> const mapRef = ref() +const position = ref() async function showSydney() { // Resolve a query into a LatLng-shaped value via the Maps API const sydney = await mapRef.value?.resolveQueryToLatLng('Sydney, Australia') // Pass it through unchanged: works for both shapes position.value = sydney } - -const position = ref() </script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/scripts/google-maps/2.api/11.overlay-view.md` around lines 140 - 152, The example declares position after it's used which can confuse readers; move the ref declaration for position so it's defined before showSydney (or before any use), e.g., declare const position = ref() above the showSydney function (and keep mapRef where it is) so showSydney, mapRef, and position are in clear top-to-bottom order.test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts (1)
126-129: Consider adding a comment explaining the triplenextTick.The three consecutive
nextTick()calls are necessary to allowuseGoogleMapsResource'swhenever()watcher and the overlay's async initialization to complete, but the rationale isn't immediately obvious.📝 Suggested documentation
// Allow useGoogleMapsResource's whenever() to fire and the overlay to attach + // Multiple ticks needed: (1) whenever() fires, (2) Promise.resolve in create, + // (3) Vue's DOM update cycle completes await nextTick() await nextTick() await nextTick()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts` around lines 126 - 129, Add an inline comment above the three consecutive await nextTick() calls in the test to explain why three ticks are required: that useGoogleMapsResource's whenever() watcher needs one microtask to run, the overlay's async initialization needs another, and a final tick ensures the overlay is attached before assertions; reference the involved symbols nextTick, useGoogleMapsResource, whenever, and the overlay initialization so future readers understand the sequencing dependency.packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue (1)
127-128: Redundant nullish coalescing.Since
defaultOpenhas an inline default oftrueon line 103, the?? truefallback on line 128 is unnecessary. The only waydefaultOpencould beundefinedat this point is if the destructuring default somehow failed, which won't happen.📝 Suggested simplification
if (open.value === undefined) - open.value = defaultOpen ?? true + open.value = defaultOpen🤖 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 127 - 128, The nullish coalescing fallback is redundant: in ScriptGoogleMapsOverlayView.vue the destructured prop/variable defaultOpen already defaults to true, so update the assignment that sets open.value (the line using "if (open.value === undefined) open.value = defaultOpen ?? true") to simply assign defaultOpen (i.e., remove "?? true"); ensure you only change the assignment expression and leave the open.value undefined check intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/content/scripts/google-maps/2.api/11.overlay-view.md`:
- Around line 140-152: The example declares position after it's used which can
confuse readers; move the ref declaration for position so it's defined before
showSydney (or before any use), e.g., declare const position = ref() above the
showSydney function (and keep mapRef where it is) so showSydney, mapRef, and
position are in clear top-to-bottom order.
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`:
- Around line 127-128: The nullish coalescing fallback is redundant: in
ScriptGoogleMapsOverlayView.vue the destructured prop/variable defaultOpen
already defaults to true, so update the assignment that sets open.value (the
line using "if (open.value === undefined) open.value = defaultOpen ?? true") to
simply assign defaultOpen (i.e., remove "?? true"); ensure you only change the
assignment expression and leave the open.value undefined check intact.
In `@test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts`:
- Around line 126-129: Add an inline comment above the three consecutive await
nextTick() calls in the test to explain why three ticks are required: that
useGoogleMapsResource's whenever() watcher needs one microtask to run, the
overlay's async initialization needs another, and a final tick ensures the
overlay is attached before assertions; reference the involved symbols nextTick,
useGoogleMapsResource, whenever, and the overlay initialization so future
readers understand the sequencing dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 777ffa0e-9285-4465-94ec-83b476d609b4
📒 Files selected for processing (4)
docs/content/scripts/google-maps/2.api/11.overlay-view.mdpackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vuepackages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.tstest/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts
1 participant
🔗 Linked issue
Related to #689 (PR D in the umbrella split-up plan).
❓ Type of change
📚 Description
Two additive changes to
<ScriptGoogleMapsOverlayView>:defaultOpenprop for the uncontrolled mode. The overlay still opens on mount by default; pass:default-open="false"to start closed without taking over the open state viav-model. Boundv-model:opencontinues to take precedence (when bound,defaultOpenis ignored).positionprop toLatLng | LatLngLiteral. Values returned by Maps APIs (e.g.resolveQueryToLatLng, marker positions) can now be passed straight through without manual conversion.Both changes are additive and backwards-compatible: existing code that omits
defaultOpenand passes aLatLngLiteralkeeps working with no behaviour change.🛠 Implementation notes
withDefaultsremoved (matches the project's preferred Vue 3.5 idiom).defineModelis kept for theopenmodel so itsdefault: undefinedopts out of Vue's boolean prop coercion. Without that opt-out an unsetopenwould coerce tofalseand break the uncontrolled default.normalizeLatLnghelper inuseGoogleMapsResourcethat detects callable.lat/.lngaccessors instead of relying oninstanceof google.maps.LatLng(mocks in tests return plain objects, so theinstanceofcheck would always be false).positionwatcher source is normalized so the watch identity is stable for bothLatLnginstances and literals.defaultOpenistrue, preserving v0 behaviour (defineModel('open', { default: undefined })opens by default).🛡 Protected behaviours
All v1 protected behaviours are untouched:
defineModel('open', { default: undefined })still treats missingv-model:openas undefined, not false (regression test ingoogle-maps-regressions.test.ts).data-state="open"/"closed"propagation on the overlay element.draw().setCenterwatcher andsetOptionsexclusion of zoom/center on the parent map.📝 Migration
None required. To opt into the new uncontrolled-closed default:
To pass a
LatLngvalue directly:const sydney = await mapRef.value?.resolveQueryToLatLng('Sydney, Australia') -<ScriptGoogleMapsOverlayView :position="{ lat: sydney.lat(), lng: sydney.lng() }" /> +<ScriptGoogleMapsOverlayView :position="sydney" />🧪 Tests
New file
test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts(usesmountSuspendedfrom@nuxt/test-utils/runtime) with:normalizeLatLng(literal pass-through, callable accessors, negative coords)defaultOpen=false,defaultOpen=true):open=true,:open=false, smoke check that mount doesn't emitupdate:open)LatLngLiteral,LatLng-shaped instance with callable accessors)The mount tests use a minimal
MockOverlayViewbase class that auto-firesonAdd+drawwhensetMapis called, mirroring the real Google Maps lifecycle just enough to exercise the SFC's draw path.All 95 google-maps-* tests pass; lint and typecheck clean.
Docs
Added "Controlled vs Uncontrolled Open State" and "Position Format" sections to
docs/content/scripts/google-maps/2.api/11.overlay-view.md.