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.

Sign up for GitHub

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

Jump to bottom

Fix - Esri Attribution not removed when VectorBasemapLayer is removed #208

Merged
merged 8 commits into from
Dec 20, 2023
Merged

Fix - Esri Attribution not removed when VectorBasemapLayer is removed #208

merged 8 commits into from
Dec 20, 2023

Conversation

BrunoCaimar
Copy link
Contributor

Fix: #136

@gavinr-maps
Copy link
Contributor

@BrunoCaimar thank you for the PR. I tested this using my demo case from the original issue:

https://jsbin.com/liriren/4/edit?html,output

  1. Click "Add Vector Tile Basemap"
  2. Click "Remove Vector Tile Basemap"
  • Expected: "Powered by esri" attribution is removed
  • Actual: the "Powered by esri" attribution is NOT removed

And it seems like it's still the same thing using your PR:
image

Can you please check this case? Thanks!

@BrunoCaimar
Copy link
Contributor Author

@gavinr-maps We will need to call removeEsriAttribution from esri-leaflet to fix that. Unfortunately, this method is not exported there: EsriUtils export.

We will need to do that before fixing this here.

I'll send a PR there, adding the method to the export. Do you need an issue there to reference this?

@BrunoCaimar BrunoCaimar mentioned this pull request Oct 27, 2023
Merged
@gavinr-maps
Copy link
Contributor

removeEsriAttribution is now exported in v3.0.12 of Esri Leaflet.

@gavinr-maps
Copy link
Contributor

Hi @BrunoCaimar, thanks a lot for working on this. Is there more work to do on this PR or is it ready for review?

@BrunoCaimar
Copy link
Contributor Author

It is ready to review.

gavinr-maps
gavinr-maps requested changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@gavinr-maps gavinr-maps left a comment

Choose a reason for hiding this comment

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

Overall looks good! One small issue:

src/VectorBasemapLayer.js Outdated Show resolved Hide resolved
@BrunoCaimar
check if function exists before calling it
674a4d8 
+ avoid error when using previous versions of esri-leaflet that do not contain the function
@gavinr-maps gavinr-maps self-requested a review December 19, 2023 18:52
gavinr
gavinr approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@gavinr gavinr left a comment

Choose a reason for hiding this comment

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

Looks good. Note it fixes the specific issue here but we do have other attribution issues. We can work on those in separate issues/PRs.

@gavinr-maps gavinr-maps merged commit 4462d7d into Esri:master Dec 20, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavinr gavinr gavinr approved these changes

@gowin20 gowin20 Awaiting requested review from gowin20

@gavinr-maps gavinr-maps Awaiting requested review from gavinr-maps

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Esri Attribution not removed when layer is removed
3 participants
@BrunoCaimar @gavinr-maps @gavinr