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

Fix setting of offset for popups. #3526

Merged
merged 1 commit into from
Jul 1, 2015
Merged

Conversation

snkashis
Copy link
Member

@snkashis snkashis commented Jun 9, 2015

Fixes #3525

@snkashis
Copy link
Member Author

snkashis commented Jun 9, 2015

Yep, my mistake.

@@ -127,7 +127,8 @@ L.Layer.include({

_popupAnchor: function (layer) {
var anchor = layer._getPopupAnchor ? layer._getPopupAnchor() : [0, 0];
return L.point(anchor).add(L.Popup.prototype.options.offset);
var offsetToAdd = layer._popup ? layer._popup.options.offset : L.Popup.prototype.options.offset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naive question: why not this._popup, as it's the popup that will be open then (or pass it as parameter on the initial call to _popupAnchor) ?

@mourner
Copy link
Member

mourner commented Jun 9, 2015

Cool! Please squash into one commit.

@snkashis
Copy link
Member Author

snkashis commented Jun 9, 2015

@mourner I don't think we can merge yet,I believe this code has introduced a slight horizontal offset that is not intended.

@patrickarlt
Copy link
Member

This will sound really really weird but I think @snkashis solution is totally correct and the offset is happening because of browser rounding in the tip of the popup. Currently the popup tip is 17x17 pixels compare this to a 16x16 pixel tip.

A radius 1 circle marker is used for reference on my retina MBP.

screen_shot_2015-07-01_at_9_40_31_am_png___1850___rgb_8__

screen_shot_2015-07-01_at_9_38_02_am_png___2460___rgb_8__

@snkashis
Copy link
Member Author

snkashis commented Jul 1, 2015

interesting @patrickarlt . what do you think @mourner , is this mergeable in current state?

@patrickarlt
Copy link
Member

Actually I took the default popup as it sits not on the this branch and measured it all.

Left edge of popup content to center of popup tip: 163px
center of popup tip to right edge of popup content: 161px

It looks like something is off by 1px somewhere I'll see if I can figure it out.

@patrickarlt
Copy link
Member

@mourner I think this is good to merge this like it is now. It addresses the problem of not using offset with my latest changes. The slight horizontal offset is still an issue in dfbd0fa which is the commit just before you merged my changes in. Check that commit and look at debug/map/markers.html

The popup is placed correctly on the market labeled "Static" and incorrectly on the marker labeled "Draggable"

screen_shot_2015-07-01_at_10_34_55_am_png___618___rgb_8__

screen_shot_2015-07-01_at_10_34_59_am_png___700___rgb_8__

@mourner
Copy link
Member

mourner commented Jul 1, 2015

@snkashis please squash into one commit and I'll merge.

@patrickarlt
Copy link
Member

I've filed an issue for the horizontal popup offset in #3586

@snkashis
Copy link
Member Author

snkashis commented Jul 1, 2015

Okay @mourner , squash done.

mourner added a commit that referenced this pull request Jul 1, 2015
Fix setting of offset for popups.
@mourner mourner merged commit 047830f into Leaflet:master Jul 1, 2015
@snkashis
Copy link
Member Author

snkashis commented Jul 1, 2015

And...I think we need to revert this because of an issue I just noticed on /debug/tests/reuse_popups.html.Offset keeps getting added - this is not correct and I guess a test for this in the future would be a good idea if we can create one
screen shot 2015-07-01 at 2 14 31 pm

@mourner
Copy link
Member

mourner commented Jul 1, 2015

Reverted.

@patrickarlt
Copy link
Member

Yup this line https://github.com/Leaflet/Leaflet/blob/master/src/layer/Layer.Popup.js#L63.

_popupAnchor needs to be called every time a popup opens to handle things like a Marker and a Polygon in the same feature group where they need to have different offsets.

@patrickarlt
Copy link
Member

The fix for this is pretty simple actually. You just have to save the originally passed offset somewhere like this._originalPopupOffset and use to calculate the popup anchor every time instead of this._popup.options.offset.

I'll have a PR for this soon.

@patrickarlt patrickarlt mentioned this pull request Jul 1, 2015
@si-the-pie
Copy link

An offset keeps getting added if the second (optional) argument to bindPopup(content, options) is left unspecified. A workaround is to use bindPopup(content, {}).

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.

5 participants