Esri / esri-leaflet

A lightweight set of tools for working with ArcGIS services in Leaflet. :rocket:
https://developers.arcgis.com/esri-leaflet/
Apache License 2.0
1.61k stars 798 forks source link

src/Util.js - custom Leaflet prefix support #1360

Closed markconnellypro closed 1 year ago

markconnellypro commented 1 year ago

When Esri Leaflet adds Powered by Esri to the attribution, it overwrites the preexisting attribution prefix. This pull request changes this process to retrieve the existing attribution prefix first, so that it can be used in lieu of the constant when overwriting and so that it can be restored in the event that all Esri layers are moved from the map display.

gavinr commented 1 year ago

Thanks for the PR. Your explanation sounds reasonable, but also I want to make sure we have a full discussion and thoughtful understanding of the problem and solution first. Given that, would you mind opening an issue for this (and you can reference this PR within that issue), clarifying the issue and the replication cases? Thank you again!

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because we're waiting on more information or details, but have not received any response. It will be closed if no further activity occurs. Thank you!

gavinr-maps commented 1 year ago

Thanks a lot for this PR @markconnellypro. I added a few additional changes:

  1. Switched the custom option on attributionControl from _originalPrefix to _esriLeafletOriginalPrefix for better namespacing.
  2. Added some unit tests.

While I was adding unit tests, I noticed a few cases where this PR is not handling the attribution correctly. Specifically those are:

  1. When you call map.attributionControl.setPrefix() after the esri layer is added. For example:

    const esriBasemapLayer = L.esri.basemapLayer("Topographic");
    esriBasemapLayer.addTo(map);
    map.attributionControl.setPrefix("aaa");
    • Expected: aaa | Powered by Esri | Esri, HERE (just the same as if you added the prefix before adding the layer)
    • Actual: aaa | Esri, HERE...
  2. When you call setPrefix() with an empty string (e.g. map.attributionControl.setPrefix('')), it should behave like other tile layers - in that the Esri attribution should still exist - it just removes any prefix that was set before. For example:

    map.attributionControl.setPrefix("");
    const esriBasemapLayer = L.esri.basemapLayer("Topographic");
    esriBasemapLayer.addTo(map);
    • Expected: Powered by Esri | Esri, HERE...
    • Actual: Leaflet | Powered by Esri | Esri, HERE...

.. so there are now 3 unit tests on this PR that are failing intentionally -- demonstrating the above 2 issues. If you have time to update the code to resolve these cases, it would be great. Otherwise I will also try to find some time to resolve. Thank you again for the PR!

markconnellypro commented 1 year ago

I pushed a commit to address the empty string issue (item 2) -- hopefully this is sufficient to fix those tests.

With respect to overwriting Powered by Esri (item 1), this appears to be a preexisting issue independent of this pull request. This is a bit at the border of my JS knowledge, but I assume to get this to work that Esri Leaflet would have to override the setPrefix method in the Control.Attribution class in Leaflet? I'm also not sure what impact such an override would have on the other functions in src/Util.js that call the setPrefix method.

markconnellypro commented 1 year ago

Closing this pull request due to #1366.