Problem/Motivation

If we close a popup then open it again, the code currently issues a new AJAX request to get the popup's content. It would be good to save the content so it can be displayed again without needing the extra AJAX request.

When the AJAX content is received and inserted into the popup, if it makes the popup larger than the initial size, the popup may be expanded outside the visible area of the map. It would be good to automatically pan the map after the AJAX request is received, so that the whole popup is visible.

Proposed resolution

After the AJAX request's content is received, we can use popup.setContent() to save the content to the popup. Then the next time the popup is opened it will display the content immediately, without needing to issue an AJAX request.

Now that the popup has the same content as is displayed on the map, we can use popup.update() to refresh the map. It will automatically pan the map if needed, to display the whole popup.

Patch to follow.

CommentFileSizeAuthor
#3 leaflet_ajax_popup_cahche_and_visibility_3258780_3.patch823 bytesitamair
#2 3258780-2.patch1.4 KBdavidhk

Comments

davidhk created an issue. See original summary.

davidhk’s picture

davidhk commented
StatusFileSize
new 3258780-2.patch1.4 KB

Patch attached.

It's been a while since I've done this, so please let me know if it has any problems.

itamair’s picture

itamair commented
Status: Active » Needs review
StatusFileSize
new leaflet_ajax_popup_cahche_and_visibility_3258780_3.patch823 bytes

Thanks @davidhk this is a nice and meaningful feature request.

The #2 patch you provided doesn't apply (it should apply inside the module itself, and you should test its application before providing it)
and its code didn't work for me at all ... (@see the attached screenshot for the why ... )

Hence I corrected it with a working one here attached.
Here some more insights for next time (look also naming conventions ...): https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

  • itamair committed 75b22f8 on 2.1.x 
    Issue #3258780 by davidhk, itamair: Ajax popups: add caching...
itamair’s picture

itamair commented

Patch #3 committed into Dev branch. Deploying a new 2.1.20 release with this ...

itamair’s picture

itamair commented
Status: Needs review » Fixed
davidhk’s picture

davidhk commented

Thanks for getting this into 2.1.20, and I'll take a look at the linked document.

jastraat’s picture

jastraat commented

Hi there, this change broke some of our custom JS code that was resizing the popup on 'popupopen' based on screen size so that we could have larger popups than the default at larger screen sizes.

Is there a better time/place to adjust popup.options.maxWidth ?

davidhk’s picture

davidhk commented

If the 'larger popups' applies to all popups, could you use one of the events leaflet.feature or leaflet.features, and process them all then?

jastraat’s picture

jastraat commented

I really appreciate you replying, but could you be more specific? Is the event you are referring to the popupopen event? An example of how to adjust leaflet.feature would be wonderful.

All the examples I've found for adjusting leaflet popup size use a variation of the one we used that broke with this change.

davidhk’s picture

davidhk commented

If you look around line 267 of leaflet.drupal.js, you'll see it triggers two events:

      // Allow others to do something with the feature that was just added to the map.
      $(document).trigger('leaflet.feature', [lFeature, feature, self]);
    }

    // Allow plugins to do things after features have been added.
    $(document).trigger('leaflet.features', [initial || false, self])

I wondered if you could use either of those events to adjust the popup.options.maxWidth, though it isn't something I've tried.

jastraat’s picture

jastraat commented

@davidhk

I stepped through the JS and set breakpoints at the two lines you mentioned in leaflet.drupal.js as well as the two lines this commit added in leaflet.drupal.js as well as within my custom popupopen event function.

I am displaying nodes within a leaflet map view.

The JS lines calling leaflet.feature are not executed at all during a view load. The popupopen JS is called correctly and when stepping through I can see my adjusted popup size until I step to the next breakpoint, but the two lines this ticket added to leaflet.drupal.js just completely overwrite any changes that the popupopen event added. That really seems incorrect.

itamair’s picture

itamair commented

@jastraat those events are being triggered in the leaflet.drupal.js if you are not using the markercluster option (https://git.drupalcode.org/project/leaflet/-/blob/2.1.x/js/leaflet.drupa...)
or in the leaflet_markercluster.drupal.js if you are using the using the markercluster option (https://git.drupalcode.org/project/leaflet/-/blob/2.1.x/modules/leaflet_...).

I didn't have the chance to inspect your issue yet and I am not sure those events (mentioned by @davidhk) could provide a workaround to your new issue ...

As this issue code is already part of the 2.1.20 code base, could you open a new Feature/Support ticket/issue with all the details of your specific use case, addressing the actual release,
and hopefully with the code gist of you that has been broken by this ... so we can better inspect a possibile solution.
I wouldn't call it a bug as it looks mostly a specific use case/alter of you, that probably will need an adjustment on the basis of the changed code base. But can also be escalated to a Bug if it proves this issue solution introduced a clear (and general, not only for you) regression in the Leaflet module ...

Thanks.

jastraat’s picture

jastraat commented

@itamair I've created a new issue #3263010: How to alter popup options

I reiterate that if there's a different recommended way to resize the popups in a leaflet map view, we are happy to approach it in a different way. So far I haven't seen another place to modify the popup options.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

vdsh’s picture

vdsh commented

It seems that this fix is incompatible with content loaded with Blazy. The first load properly loads my Blazy image, but subsequent closing of the popup/reopening it only provides the Blazy throbber. If I remove the content added by this patch
e.popup.setContent(element.innerHTML);
and
e.popup.update();
Then my image is properly reloaded on every click of the popup (but I am loosing cacheability ...)

Note: I found #3068719: Attachments and Cacheability dropped for Leaflet (embeded|ajax) popups which is slightly related