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

Fix several bugs and add basemap selector to core. #1735

Merged
merged 9 commits into from
Sep 28, 2023
Merged

Conversation

naschmitz
Copy link
Collaborator

@naschmitz naschmitz commented Sep 26, 2023

The basemap selector for geemap should be unchanged.

The basemap selector for core actually swaps out layer[0]. Users can manually add additional basemap layers if they choose.

@naschmitz naschmitz requested a review from giswqs September 26, 2023 19:55
@github-actions
Copy link

github-actions bot commented Sep 26, 2023

@github-actions github-actions bot temporarily deployed to pull request September 26, 2023 20:20 Inactive
@giswqs
Copy link
Member

giswqs commented Sep 28, 2023

The basemap selector does not seems to show up on the map for the core module. Investigating.

image

@github-actions github-actions bot temporarily deployed to pull request September 28, 2023 02:19 Inactive
@giswqs
Copy link
Member

giswqs commented Sep 28, 2023

The m.add('basemap_selector') method does not have effect either for the geemap module. It can only be activated by clicking on the toolbar icon.

image

@naschmitz
Copy link
Collaborator Author

Screenshot 2023-09-27 at 10 27 43 PM

Are you testing on the correct branch? It's appearing for me.

@giswqs
Copy link
Member

giswqs commented Sep 28, 2023

This is a werid issue. I already checked out the correct branch.

image

@giswqs
Copy link
Member

giswqs commented Sep 28, 2023

Never mind. I figured it out. I am on a travel laptop. Forgot to use pip install -e .

@giswqs
Copy link
Member

giswqs commented Sep 28, 2023

It seems working fine when there are other layers besides the basemap. If there is only the basemap layer, changing the basemap will throw an error.

import geemap.core as geemap
m = geemap.Map()
m.add('basemap_selector')
m

image

@giswqs
Copy link
Member

giswqs commented Sep 28, 2023

Just discovered another bug. The All layers on/off toggle is not working properly. This is probably caused by a previous PR. Not this PR.

Screen.Recording.2023-09-27.at.11.16.27.PM.mov

geemap/core.py Outdated
ipyleaflet.TileLayer(
url=basemap["url"],
name=basemap["name"],
max_zoom=basemap.get("max_zoom", 22),
Copy link
Member

Choose a reason for hiding this comment

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

recommend minimum max zoom 24

@giswqs
Copy link
Member

giswqs commented Sep 28, 2023

This seems an unexpect behavior (or maybe a bug) of ipyleaflet. When there is only one layer on the map, m.remove_layer() does not work. However, m.clear_layers() works fine.

import ipyleaflet
m = ipyleaflet.Map(center=(0, 0), zoom=2)
m.remove_layer(m.layers[0]) # not working
m.clear_layers() # working

@naschmitz
Copy link
Collaborator Author

Thanks for finding these issues. Looking into them now.

@giswqs
Copy link
Member

giswqs commented Sep 28, 2023

Thanks for the quick fix. It works now.

@giswqs giswqs self-requested a review September 28, 2023 03:53
@giswqs giswqs merged commit 4fffb1f into master Sep 28, 2023
@giswqs giswqs deleted the nschmitz-basemaps branch September 28, 2023 03:53
@github-actions github-actions bot temporarily deployed to pull request September 28, 2023 03:54 Inactive
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.

2 participants