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

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

Closed
wants to merge 8 commits into from
Closed

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

wants to merge 8 commits into from

Conversation

markconnellypro
Copy link

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
Copy link
Contributor

gavinr commented May 8, 2023

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
Copy link

stale bot commented Jun 8, 2023

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!

@stale stale bot added the stale Closed due to inactivity. label Jun 8, 2023
@gavinr gavinr removed Comments Needed stale Closed due to inactivity. labels Jun 9, 2023
@gavinr-maps
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

Closing this pull request due to #1366.

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.

None yet

3 participants