search

backdrop-contrib / ip_geoloc

IP Geolocation Views & Maps for Backdrop CMS.
GNU General Public License v2.0
0 stars 2 forks source link

Libraries dependency not fully removed yet #2

Open laryn opened 4 years ago

laryn commented 4 years ago

Error: Call to undefined function libraries_get_path() in ip_geoloc_plugin_style_leaflet->_add_check_boxes() (line 403 of /path/modules/ip_geoloc/views/ip_geoloc_plugin_style_leaflet.inc).

laryn commented 1 year ago

Error: Call to undefined function leaflet_markercluster_get_library_path() in ip_geoloc_requirements() (line 77 of /app/modules/contrib/ip_geoloc/ip_geoloc.install).

laryn commented 5 months ago

This may also be partly addressed by @AlexHoebart-ICPDR's work in https://github.com/backdrop-contrib/ip_geoloc/pull/9 -- although the leaflet_markercluster issues may need additional work.

AlexHoebart-ICPDR commented 5 months ago

I'm not using the markercluster, but as it is already included by the leaflet_markercluster module, some parts here could probably be removed.

I do use the mini-map plugin provided with ip_geoloc and I could package it with this module. However, it might be also worth to consider to package it with the leaflet module, as it is not very specific to the ip_geoloc module. It's something which could be useful for any kind of map. What do you think @indigoxela? The plugin is available here: https://github.com/Norkart/Leaflet-MiniMap?tab=readme-ov-file Is it the usual approach to keep as much as possible the original scope of the Drupal modules, or could we improve here?

indigoxela commented 5 months ago

The structure of the Leaflet module for B differs quite from the D7 module.

The markercluster module's included in the Leaflet module and the library's on, as soon as the module's enabled ("leaflet_markercluster"). The fullscreen JS ships with the leaflet library (Leaflet.fullscreen.js). No need for an extra check, nor libraries_get_path. See leaflet_library_info(). Not sure if the toggling in the views style actually works. I never used ip_geoloc.

FTR: The set of plugins included in the Leaflet module wasn't my decision, but that has happened, before I took over maintainership. I worked with what I found.

I do use the mini-map plugin provided with ip_geoloc and I could package it with this module. However, it might be also worth to consider to package it with the leaflet module

TBH, I'm hesitant to add yet another 3rd party Leaflet plugin to the module I maintain. It's just another technical debt for me. The plugin hasn't been updated in years. And given, that Leaflet 2.x is the next version of the main lib, and that version will come with several breaking changes, which will be a challenge for me, already... :wink: Better not.

AlexHoebart-ICPDR commented 2 months ago

Markercluster is done in #9, Mini-map still needs to be packaged.