Offset tooltip popup so it fits inside the map bounds #9819
Conversation
|
Can one of the admins verify this patch? |
|
jenkins, test this |
thomasneirynck
added
Feature:Visualizations
|
Anyone else think that the tooltip display of the features geometry coordinates as an object is hard to read? I think I like this better. Would it be OK to add this change in as well or create another issue? |
|
@JacobBrandt the new info-box is more crisp, you're right. afaic, it's fine to add that to this PR. |
|
Hi @JacobBrandt, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile? |
|
@karmi I re-signed with my current e-mail. I no longer have the other one used in the commit. |
JacobBrandt
force-pushed
the
tilemap_tooltips_cutoff_master
branch
from
e35c20d to
7924e45
Compare
There was a problem hiding this comment.
thanks @JacobBrandt
I like the improvement to the label contents and we should merge those in IMO.
I don't think we should apply this fix this way though. For a few reasons:
- we have to rely on a clever trick to measure the balloon contents
- we have to rely on implementation details of Leaflet to retrieve the DOM-node and then measure it.
- we also need to couple styling info from our CSS so we can do the offsets appropriately (the 16 pixels).
IMHO, keeping a tooltip inside the map is an upstream issue with Leaflet, and it should be addressed there. I understand this work-around does add value to Kibana, but what we're really doing is patching some Leaflet limitations inside Kibana. There are other places in Kibana where we depend on 'hacks' to achieve some desired effect (e.g. desaturating the map colors), but those shouldn't really have sneaked in there to begin with. So we should avoid this going forward.
If you're interested to work further on this, I would take out the re-positioning logic for the tooltip, and just keep the improvement to the contents.
| lat: feature.geometry.coordinates[1], | ||
| lon: feature.geometry.coordinates[0] | ||
| }) | ||
| label: 'Latitude', |
There was a problem hiding this comment.
I think this is an improvement. Crisper and more legible.
| const popupDistanceToRightEdge = Math.abs(latLng.lng - east) * xscale; | ||
| let widthOffset = 0; | ||
| if (popupDistanceToLeftEdge < popupWidth / 2) { | ||
| widthOffset = popupWidth / 2; |
There was a problem hiding this comment.
for the pop-up to attach to the point, we'd probably want:
widthOffset = popupWidth / 4 + 16;
Similar for right side:
widthOffset = -popupWidth / 4 - 16;
|
|
||
| // We need to create the popup first to determine how tall it will be. Give | ||
| // it an out of map bounds offset as we don't want to see this one. It will | ||
| // get replaced on the next popup creation where we apply the correct offset |
| .setLatLng(latLng) | ||
| .setContent(content) | ||
| .openOn(this.map); | ||
| const popupHeight = popup._contentNode.clientHeight; |
There was a problem hiding this comment.
We probably shouldn't introduce dependencies on internal implementation details of 3rd party libraries.
|
@thomasneirynck Yea I see what your saying. I did have to rely on knowing some inner workings of Leaflet which I wasn't really a fan of. Although I do question that if this is really an upstream Leaflet issue (which I do think it is, don't get me wrong) than why was the issue #8799 given bug and P3 labels and then also referenced by other issues for instance #8944? I'd rather not waste my time on Kibana issues that are never going to get merged/fixed as Kibana code changes. Could there be some kind of purge effort for issues you guys never intend to fix and instead are waiting for a fix from some 3rd party vendor? |
|
So several ways to handle the content changes. Which way do you prefer? I'm fine with whatever you pick just let me know.
|
|
@JacobBrandt you're right, this can be more clear. Those P-labels are often added before we know what the actual issue is, or what the fix would be. They are an indication of how important somebody thinks it is. And as this issue affects map-users in Kibana, it was added to that meta-ticket too, much like most other map issues. I understand the confusion though. One thing to do, is ping on the issue that you're working on a fix. That will alert people for quicker feedback. That said, for this particular issue, it's really only by working through the problem that it became clear Leaflet does not have sufficient public API to accurately determine label bounds, so not sure if would have been revealed during an initial issue triage. Since the discussion here is quite lengthy, I'd create a new PR with just the label change, thx |
|
Closing, will get new PR for label improvement |
|
@thomasneirynck Thanks for the explanation. That makes a lot of sense. I'll get the new PR done later tonight. |
Fixes #8799