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

Add layer loading spinner and remove layer button #1983

Merged
merged 12 commits into from
Jun 12, 2024
Merged

Add layer loading spinner and remove layer button #1983

merged 12 commits into from
Jun 12, 2024

Conversation

giswqs
Copy link
Member

@giswqs giswqs commented Apr 21, 2024

This PR adds a layer loading spinner and remove layer button to the layer manager. The layer widget padding is slightly reduced from 8px to 4x to make room for the two newly added buttons.

layer_loading_spinner.mp4

Copy link

github-actions bot commented Apr 21, 2024

@github-actions github-actions bot temporarily deployed to pull request April 21, 2024 02:03 Inactive
@giswqs giswqs linked an issue Apr 21, 2024 that may be closed by this pull request
@giswqs
Copy link
Member Author

giswqs commented Apr 21, 2024

Just realized that I forgot to add a confirmation dialog when removing a layer. Will add that later

@giswqs
Copy link
Member Author

giswqs commented May 10, 2024

I have added the confirmation dialog when removing a layer. The PR is ready for review now.

spinner.mp4

@github-actions github-actions bot temporarily deployed to pull request May 10, 2024 04:35 Inactive
Copy link
Collaborator

@naschmitz naschmitz left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for taking this on!

@@ -6,6 +6,7 @@
from IPython.core.display import HTML, display

import ee
import ipyleaflet
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't import ipyleaflet in map_widgets. This file is supposed to be agnostic to map implementation. Two possible solutions are listed below.

  1. The preferred option is to avoid this problem entirely by showing the confirmation dialog above (or inside) the existing widget. It could look like another row that's added below the row scheduled for deletion or a dialog that appears over everything.

  2. Build this new "confirmation" widget as any of the other core widgets and have the widget control code live in core. The new widget needs some sort of "on_confirm_deletion" event that the code code must set. Example:

    def _add_basemap_selector(self, position: str, **kwargs) -> None:

Copy link
Member Author

Choose a reason for hiding this comment

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

I have implemented the preferred option 1:

another row that's added below the row scheduled for deletion

remove.layer.mp4

geemap/map_widgets.py Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request May 15, 2024 03:12 Inactive
@github-actions github-actions bot temporarily deployed to pull request May 15, 2024 04:04 Inactive
@github-actions github-actions bot temporarily deployed to pull request May 25, 2024 03:26 Inactive
@giswqs
Copy link
Member Author

giswqs commented May 25, 2024

The remove layer confirmatition widget is now being displayed beneath the layer row in the layer manager. See the new demo:

remove.layer.mp4

@github-actions github-actions bot temporarily deployed to pull request May 25, 2024 04:36 Inactive
@giswqs
Copy link
Member Author

giswqs commented May 25, 2024

@naschmitz @jdbcode It is ready for review now.

@github-actions github-actions bot temporarily deployed to pull request May 27, 2024 22:51 Inactive
@naschmitz naschmitz merged commit 7bf4e3d into master Jun 12, 2024
12 of 13 checks passed
@naschmitz naschmitz deleted the spinner branch June 12, 2024 16:18
@github-actions github-actions bot temporarily deployed to pull request June 12, 2024 16:26 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.

Layer loading spinner proof-of-concept
2 participants