Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popup/Tooltip content function: allow return undefined #6599

Conversation

johnd0e
Copy link
Collaborator

@johnd0e johnd0e commented Apr 15, 2019

In this case Popup/Tooltip is not shown.

Useful when only some sublayers need popups:

  • previously: it was necessary to attach independent popups to each sublayer
  • now: it's enough to have common content function attached to parent layer

d8a4ba0

diff --git a/src/layer/DivOverlay.js b/src/layer/DivOverlay.js
index efd76f1bba..0005ac6fdc 100644
--- a/src/layer/DivOverlay.js
+++ b/src/layer/DivOverlay.js
@@ -164,7 +164,9 @@ export var DivOverlay = Layer.extend({
 		var node = this._contentNode;
 		var content = (typeof this._content === 'function') ? this._content(this._source || this) : this._content;
 
-		if (typeof content === 'string') {
+		if (!content) {
+			return;
+		} else if (typeof content === 'string') {
 			node.innerHTML = content;
 		} else {
 			while (node.hasChildNodes()) {

This change is Reviewable

@johnd0e
Copy link
Collaborator Author

johnd0e commented Apr 15, 2019

Well, I was too optimistic.
It appeared that there much more deep changes needed, as function is called after Popup/Tooltip is already added on map.

So I am not sure how to deal with it best:

  • simple approach: just close empty popup.
  • complex approach: change whole popup processing chain in order to check content before popup opened.

@johnd0e johnd0e force-pushed the content-function-return-allow-undefined branch 2 times, most recently from d8a4ba0 to 9e0ec58 Compare April 15, 2019 07:55
@johnd0e
Copy link
Collaborator Author

johnd0e commented Apr 15, 2019

  • simple approach: just close empty popup.

9e0ec58

diff --git a/src/layer/DivOverlay.js b/src/layer/DivOverlay.js
index efd76f1bba..9a82ffbce0 100644
--- a/src/layer/DivOverlay.js
+++ b/src/layer/DivOverlay.js
@@ -113,7 +113,10 @@ export var DivOverlay = Layer.extend({
 
 		this._container.style.visibility = 'hidden';
 
-		this._updateContent();
+		if (!this._updateContent()) {
+			this._close();
+			return;
+		}
 		this._updateLayout();
 		this._updatePosition();
 
@@ -164,7 +167,9 @@ export var DivOverlay = Layer.extend({
 		var node = this._contentNode;
 		var content = (typeof this._content === 'function') ? this._content(this._source || this) : this._content;
 
-		if (typeof content === 'string') {
+		if (!content) {
+			return;
+		} else if (typeof content === 'string') {
 			node.innerHTML = content;
 		} else {
 			while (node.hasChildNodes()) {
@@ -173,6 +178,7 @@ export var DivOverlay = Layer.extend({
 			node.appendChild(content);
 		}
 		this.fire('contentupdate');
+		return true;
 	},
 
 	_updatePosition: function () {

Although this works for me, but some tests in PhantomJS failed.
Waiting for advice.

In this case Popup/Tooltip is not shown.

Useful when only some sublayers need popups:
- previously: it was necessary to attach independent popups to each sublayer
- now: it's enough to have common content function attached to parent layer
@johnd0e johnd0e force-pushed the content-function-return-allow-undefined branch from 444ef1f to df506ca Compare April 18, 2019 11:27
@johnd0e
Copy link
Collaborator Author

johnd0e commented Apr 18, 2019

Patch reworked, now all tests are happy.
Known drawback: content function now called twice.
I suppose it's acceptable, as we gain much more with new feature.
Or I will be happy to remake the patch, if anyone point me more proper place to fix.

N.B.
openTooltip and openPopup contain pretty the same code, I suppose it'd be better refactored: #6613 (place common function in DivOverlay)

@johnd0e
Copy link
Collaborator Author

johnd0e commented May 9, 2019

Close in favor of #6651

@johnd0e johnd0e closed this May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant