Use overlayLayer and overlayMouseTarget #518

ashleahhill · 2019-10-07T13:29:13Z

Currently, the lib is drawing the visible target on the overlayMouseTarget layer instead of the overlayLayer. My understanding of how these layers are supposed to work is: visuals on overlay layer, synced, invisible eventTargets on the overlayMouseTarget layer.

I came to this conclusion by picking apart the default map markers and this documentation: https://developers.google.com/maps/documentation/javascript/reference/overlay-view#MapPanes

With this setup, the z-index logic can remain simple and the clusters will continue to show up over pins but under other UI.

fixes #506

googlebot · 2019-10-07T13:29:33Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

garylittleRLP · 2019-10-07T15:03:00Z

markerclustererplus/src/markerclusterer.js

@@ -120,31 +121,37 @@ ClusterIcon.prototype.onAdd = function () {

  this.div_ = document.createElement("div");
  this.div_.className = this.className_;
+  this.eventDiv_ = this.div_.cloneNode();
+  this.eventDiv_.className += ' event';


Any particular reason for adding the "event" class name to the overlayMouseTarget div? Shouldn't it be exactly the same class as the overlayLayer div?

I'd like to have a way to differentiate the visible div, div.classname and the event div, div.classname.event . The same class configured by the user is on both divs, but event is added so that you can use a :not or other selector.

I guess I could use a more BEM friendly div.classname.div.classname--event if you'd prefer?

The two divs should be absolutely identical, so I think it's best if the overlayMouseTarget div has exactly the same class as the overlayLayer div.

garylittleRLP · 2019-10-07T15:08:47Z

markerclustererplus/src/markerclusterer.js

+    this.syncDivStyle({
+      title: this.div_.title,
+    });


Don't you have to synch the display style as well?

My intention was only to set the props that the code sets on the visible div. Line 388 copies all of the css set on this.div_

Edit: for grammar, typo

garylittleRLP · 2019-10-07T15:19:24Z

markerclustererplus/src/markerclusterer.js

@@ -286,6 +298,10 @@ ClusterIcon.prototype.show = function () {
      this.div_.title = this.sums_.title;


Just before this line you should also be setting the innerHTML of the overlayMouseTarget div to the same as that of the overlayLayer div. This ensures the overlayMouseTarget div has exactly the same size as the overlayLayer div.

Why wouldn't copying the height and width from the overlayDiv work without duplicating innerHTML?

I'm open to setting it and adding opacity: 0 to line 388; it just seems unnecessary.

I'm just being cautious.

Syncing innerHTML to event div and setting opacity when css is updated (in syncDivStyle method)

ashleahhill · 2019-10-07T16:45:55Z

markerclustererplus/src/markerclusterer.js

+ */
+ClusterIcon.prototype.syncDivStyle = function (vars) {
+  if (vars) {
+    Object.assign(this.eventDiv_, vars);


This line is used to sync any other DOMElement props outside of CSS. The only property I found that needed this was the title.

ashleahhill · 2019-10-07T16:47:39Z

@googlebot I signed it!

ashleahhill · 2019-10-07T16:51:11Z

@googlebot I signed it!

googlebot · 2019-10-07T16:51:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

ashleahhill · 2019-10-07T17:17:59Z

markerclustererplus/src/markerclusterer.js

+    Object.assign(this.eventDiv_, vars);
+  }
+  if (this.eventDiv_ && this.div_) {
+    this.eventDiv_.style.cssText = this.div_.style.cssText;


The overlayDiv, this.div_, is positioned and resized using CSS. By copying the cssText we copy the same height, width, z-index, etc. without having to specify each.

ashleahhill · 2019-10-16T18:52:30Z

I noticed that there's a big ES6 rewrite happening. Is this fix still necessary?

fixes googlemaps#506

jpoehnelt · 2019-10-17T18:20:49Z

@ashleahhill I'm not sure when I will get to rewriting this one. I'll defer to @garylittleRLP for the comments already made here.

ashleahhill · 2019-10-18T12:32:26Z

I'm also fine with rebasing this off of the 3.0 work or opening another PR for 3.0.

jpoehnelt · 2019-10-30T00:23:51Z

closing in favor of #558

jpoehnelt · 2019-11-05T16:29:22Z

@ashleahhill see #558 (comment) to consent to your commit being merged

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Oct 7, 2019

garylittleRLP reviewed Oct 7, 2019

View reviewed changes

ashleahhill commented Oct 7, 2019

View reviewed changes

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Oct 7, 2019

ashleahhill commented Oct 7, 2019

View reviewed changes

Use overlayLayer and overlayMouseTarget

4519af5

fixes googlemaps#506

ashleahhill force-pushed the master branch from 20690c0 to 4519af5 Compare October 16, 2019 19:04

jpoehnelt mentioned this pull request Oct 30, 2019

add event overlay #558

Open

jpoehnelt closed this Oct 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use overlayLayer and overlayMouseTarget #518

Use overlayLayer and overlayMouseTarget #518

ashleahhill commented Oct 7, 2019

googlebot commented Oct 7, 2019

garylittleRLP Oct 7, 2019 •

edited

Loading

ashleahhill Oct 7, 2019

garylittleRLP Oct 8, 2019

ashleahhill Oct 16, 2019

garylittleRLP Oct 7, 2019

ashleahhill Oct 7, 2019 •

edited

Loading

ashleahhill Oct 16, 2019

garylittleRLP Oct 7, 2019

ashleahhill Oct 7, 2019

garylittleRLP Oct 8, 2019

ashleahhill Oct 16, 2019 •

edited

Loading

ashleahhill Oct 7, 2019 •

edited

Loading

ashleahhill commented Oct 7, 2019

ashleahhill commented Oct 7, 2019

googlebot commented Oct 7, 2019

ashleahhill Oct 7, 2019

ashleahhill commented Oct 16, 2019

jpoehnelt commented Oct 17, 2019

ashleahhill commented Oct 18, 2019

jpoehnelt commented Oct 30, 2019

jpoehnelt commented Nov 5, 2019

		@@ -286,6 +298,10 @@ ClusterIcon.prototype.show = function () {
		this.div_.title = this.sums_.title;

Use overlayLayer and overlayMouseTarget #518

Use overlayLayer and overlayMouseTarget #518

Conversation

ashleahhill commented Oct 7, 2019

googlebot commented Oct 7, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

garylittleRLP Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleahhill Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleahhill Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

ashleahhill Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

ashleahhill commented Oct 7, 2019

ashleahhill commented Oct 7, 2019

googlebot commented Oct 7, 2019

Choose a reason for hiding this comment

ashleahhill commented Oct 16, 2019

jpoehnelt commented Oct 17, 2019

ashleahhill commented Oct 18, 2019

jpoehnelt commented Oct 30, 2019

jpoehnelt commented Nov 5, 2019

garylittleRLP Oct 7, 2019 •

edited

Loading

ashleahhill Oct 7, 2019 •

edited

Loading

ashleahhill Oct 16, 2019 •

edited

Loading

ashleahhill Oct 7, 2019 •

edited

Loading