Add advanced markers support#48
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a ChangesAdvanced marker support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 7
🤖 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 `@demo/advanced-markers-clustering.html`:
- Around line 42-60: Move the marker-generation loop into a one-time dom-change
listener on the surrounding template so it runs only after dom-bind stamps `#map`.
Resolve `#map` inside that callback before appending the generated markers,
preserving the existing deterministic positions and marker properties.
In `@google-map-advanced-marker.js`:
- Around line 429-436: Update the click handling around openInfoHandler_ and the
corresponding clickEvents listener so long-press suppression is evaluated by one
shared dispatcher before either listener acts. Ensure _suppressNextClick is
cleared only after all handlers for that click have been prevented from
forwarding, so enabling both InfoWindow and clickEvents still swallows the
synthetic click.
- Around line 296-309: Update the detached()/attached() lifecycle so detaching
does not leave the marker with missing subscriptions or unreleasable Maps
listener handles. Either preserve _listeners and DOM/content subscriptions
during detachment, or have attached() fully recreate them—including
mouse/double-click forwarding, content observation, and listener
bookkeeping—before restoring the marker with setMap(this.map).
- Around line 390-402: Update _mapChanged() and _mapReady() to use a
map/generation token for each marker-construction attempt. Capture the current
token before asynchronous marker loading, and have _mapReady() abort when the
token no longer matches the latest map change; ensure only the still-current
attempt constructs and attaches a marker.
- Around line 378-381: Update _collisionBehaviorChanged() so it always
synchronizes the marker when collisionBehavior changes: assign the mapped
CollisionBehavior value when configured, and reset the marker’s
collisionBehavior when the property is null or cleared instead of leaving the
previous value active.
- Around line 443-449: Update the this.info cleanup branch to close the existing
InfoWindow before removing its listeners and setting this.info to null. Preserve
the current listener removal, clickable-state update, and empty-window cleanup
behavior.
In `@README.md`:
- Line 15: Update the README import and usage examples to include the
advanced-marker component alongside the legacy marker example, and configure the
map with the required map-id attribute. Keep the existing legacy example intact
while making the advertised AdvancedMarkerElement feature directly actionable.
🪄 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: d3901360-d620-40c4-9016-43621c0e5283
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.mddemo/advanced-markers-clustering.htmldemo/advanced-markers.htmlgoogle-map-advanced-marker.jsgoogle-map-elements.jsgoogle-map-marker.jsgoogle-map.jspackage.json
6a94172 to
0944064
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@google-map-advanced-marker.js`:
- Around line 296-303: Update detached() and the marker replacement cleanup
around _touchTimer to cancel any pending long-press timer when the marker is
removed or replaced, clear its reference, and preserve the existing listener and
observer cleanup behavior.
- Around line 537-541: Update the initialization flow around _openChanged() so
it does not close an InfoWindow or emit google-map-marker-close when open is
initially false and the window was never opened. Only invoke the open-state
change handling when the component has an established/open state that requires
synchronization, while preserving the existing content, click, drag, and mouse
event setup.
- Around line 513-523: Update the AdvancedMarkerElement construction to parse
latitude and longitude first, and include the position option only when both
parsed values are finite. When either coordinate is null, invalid, or
non-finite, omit position entirely while preserving the other marker options.
🪄 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: aba9bd52-4c41-4f4a-82ec-4347a35f0771
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.mddemo/advanced-markers-clustering.htmldemo/advanced-markers.htmlgoogle-map-advanced-marker.jsgoogle-map-elements.jsgoogle-map-marker.jsgoogle-map.jspackage.json
0944064 to
edd7428
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 57 minutes. |
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 `@google-map-advanced-marker.js`:
- Around line 721-774: Update _setupTouchAndHold’s pointerdown handler to start
the long-press timer only for primary touch or other touch-like pointers,
ignoring mouse and non-primary pointer events. Add pointercancel to the
clearTimer cancellation listeners so cancelled gestures always stop the timer.
- Around line 315-321: Update _updatePosition() to parse latitude and longitude
once, validate that both coordinates are finite, and assign marker.position to
the coordinate object only when valid; otherwise assign null so missing or
invalid values clear any existing pin.
🪄 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: 59d0bec6-7c82-4d3d-9621-512dab5b9443
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.mddemo/advanced-markers-clustering.htmldemo/advanced-markers.htmlgoogle-map-advanced-marker.jsgoogle-map-elements.jsgoogle-map-marker.jsgoogle-map.jspackage.json
edd7428 to
e46cf64
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@google-map-advanced-marker.js`:
- Around line 324-337: Update _clickEventsChanged() to cancel any pending
touch-and-hold timer when clickEvents becomes false, before or alongside
clearing the DOM listeners. Reuse the existing touch-hold cleanup state or
method used by _setupTouchAndHold(), ensuring no delayed rightclick event is
emitted after click events are disabled.
- Around line 451-461: Update the InfoWindow creation flow in the
content-handling method so that when dynamic content creates this.info while
open is already true, it immediately opens the InfoWindow using the marker’s
existing open behavior. Preserve the current click-handler setup and avoid
changing behavior when open is false.
- Around line 694-697: Update getPosition() to validate parsed latitude and
longitude before constructing a LatLng, returning null when either coordinate is
missing or non-finite. In _loadMarkersCluster(), filter this.markers so only
markers with valid positions are passed to MarkerClusterer, ensuring
Supercluster never receives unpositioned markers.
🪄 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: 6aa6f13e-5308-41f7-8761-07b37de28f0f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.mddemo/advanced-markers-clustering.htmldemo/advanced-markers.htmlgoogle-map-advanced-marker.jsgoogle-map-elements.jsgoogle-map-marker.jsgoogle-map.jspackage.json
e46cf64 to
0af5917
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
demo/advanced-markers-clustering.html (1)
55-69: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch marker appends using a
DocumentFragment.Appending markers to the
mapelement individually inside the loop can trigger multiple synchronous layouts and unnecessarily fire the<google-map>'s internal mutation observer 60 times.Using a
DocumentFragmentwill batch the insertions, allowing the map component to process all added markers in a single observer execution.⚡ Proposed optimization
+ const fragment = document.createDocumentFragment(); for (let i = 0; i < 60; i++) { const marker = document.createElement('google-map-advanced-marker'); marker.setAttribute('slot', 'markers'); marker.latitude = CENTER.lat + (random() - 0.5) * 0.5; marker.longitude = CENTER.lng + (random() - 0.5) * 0.5; marker.title = 'Marker ' + (i + 1); marker.background = COLORS[i % COLORS.length]; marker.glyphColor = '`#FFFFFF`'; marker.glyph = String(i + 1); marker.clickEvents = true; marker.addEventListener('google-map-marker-click', function(e) { console.log(this.title + ' clicked', e.detail.latLng.lat(), e.detail.latLng.lng()); }); - map.appendChild(marker); + fragment.appendChild(marker); } + map.appendChild(fragment);🤖 Prompt for 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. In `@demo/advanced-markers-clustering.html` around lines 55 - 69, Update the marker creation loop in the clustering demo to append each new marker to a DocumentFragment instead of directly to map, then append the populated fragment to map once after the loop. Preserve all marker configuration and click handling unchanged.
🤖 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 `@google-map-advanced-marker.js`:
- Around line 557-565: Prevent the initial setup path from calling _openChanged
when open is already true, since _contentChanged has already created and opened
the InfoWindow and dispatched the event. Preserve _openChanged for later
open-state changes so each transition still emits exactly once.
- Around line 559-567: Move the setupDragHandler_().bind(this)() call before
_dragEventsChanged() during initialization. Preserve the existing mouse-event
handling and conditional _openChanged() behavior, ensuring the drag handler is
registered before the first dragend event can be forwarded so latitude and
longitude are updated first.
In `@google-map.js`:
- Around line 966-968: Update the customRenderer handling to remove eval()
entirely and accept only a renderer object supplied through property binding. In
the relevant renderer-options logic, assign customRenderer directly when it is
an appropriate object, while rejecting or ignoring string values so no
user-provided JavaScript is executed.
---
Nitpick comments:
In `@demo/advanced-markers-clustering.html`:
- Around line 55-69: Update the marker creation loop in the clustering demo to
append each new marker to a DocumentFragment instead of directly to map, then
append the populated fragment to map once after the loop. Preserve all marker
configuration and click handling unchanged.
🪄 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: d8db2012-10a9-4c51-b96b-56003f9a41a1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.mddemo/advanced-markers-clustering.htmldemo/advanced-markers.htmlgoogle-map-advanced-marker.jsgoogle-map-elements.jsgoogle-map-marker.jsgoogle-map.jspackage.json
0af5917 to
2063fa1
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Code Review FindingsThe following review was generated by Claude. 🔴 1. Markers positioned after insertion are permanently dropped from clusters —
|
github-project-automation
Bot
moved this from To Do
to In Progress
in Flowing Code Addons
|
Thanks for the review — all three are addressed (pushed as WIP commits): 1. Markers positioned after insertion dropped from clusters — b11e5c5. The map now listens for a 2. Stale 3. Cached |
All fixed, see #48 (comment) |
2 participants
Adds a new
google-map-advanced-markerelement based onAdvancedMarkerElement, Google's replacement for the deprecatedgoogle.maps.Marker. Part of FlowingCode/GoogleMapsAddon#121.The element supports pin customization (background, border color, glyph, scale). The original
google-map-markeris kept: advanced markers require amapIdand don't support some classic features (Symbolicons, labels, animation), so replacing it would break existing users. Per Google's deprecations page,google.maps.Markeris "not scheduled to be discontinued" and keeps working. A doc note now points new development to the new element.Added new demos:
demo/advanced-markers.htmlanddemo/advanced-markers-clustering.html.Summary by CodeRabbit
google-map-advanced-marker(based onAdvancedMarkerElement) with support for styled/icon/image and custom HTML content, dragging, InfoWindow open/close, and configurable event forwarding (including touch long-press “right-click”).fit-to-markersto consider only markers with valid latitude/longitude, with correct single-marker centering.map-id).