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

Default the layer manager to be opened on map initialization and other toolbar tweaks #2039

Merged
merged 7 commits into from
Jun 14, 2024

Conversation

sufyanAbbasi
Copy link
Collaborator

  • The toolbar will now always show both the layer and toolbar buttons.
  • On core map initialization, the layer manager will be opened by default.
  • Right justified the buttons so that they didn't shift places when the layer manager is opened.
  • The hover to open toolbar feature was kept in order to not disrupt existing workflows, however I noticed that this feature only works in Jupyter Lab and not Colab.

See: https://colab.research.google.com/drive/1HHQomXhy-vS68qiMu6AOhDmrsLq6OZwe#scrollTo=IouG0AzwPFID

Screenshot 2024-06-11 at 8 49 42 PM Screenshot 2024-06-11 at 8 49 56 PM Screenshot 2024-06-11 at 8 50 21 PM

@giswqs
Copy link
Member

giswqs commented Jun 12, 2024

I am aware of this issue. It is related to ipyevents. I believe it does not support Colab. Need some investigation.

The hover to open toolbar feature was kept in order to not disrupt existing workflows, however I noticed that this feature only works in Jupyter Lab and not Colab.

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.

Could you add unit tests if it's not too much trouble?

@sufyanAbbasi
Copy link
Collaborator Author

Could you add unit tests if it's not too much trouble?

No trouble at all! Done.

@sufyanAbbasi
Copy link
Collaborator Author

I am aware of this issue. It is related to ipyevents. I believe it does not support Colab. Need some investigation.

Gotcha, thanks for raising that! I'm leaning towards removing the hover effect for two reasons: 1) it's highly susceptible for jitter, and 2) it only opens the toolbar on hover since it would be hard to detect which button is being hovered (can use pixel calculations maybe?), so it might feel weird to users that only one of the two buttons are openable on hover, if that makes sense.
Screenshot 2024-06-12 at 5 42 27 PM

LMK what you prefer!

@giswqs
Copy link
Member

giswqs commented Jun 13, 2024

@sufyanAbbasi Makes sense. Let's remove the hovering effect. We can potentially remove the ipyevent dependency completely after removing the hovering effect on the data search button in the upper-left corner. I can do that in another PR.

@giswqs
Copy link
Member

giswqs commented Jun 13, 2024

I noticed one minor artifact: when clicking on the layer button, it shows the toolbar buttons briefly before showing the layer manager.

Peek 2024-06-12 22-40

@sufyanAbbasi
Copy link
Collaborator Author

Let's remove the hovering effect.

On it!

it shows the toolbar buttons briefly before showing the layer manager.

Ah, good eye! Let me figure out what's going on there.

@sufyanAbbasi
Copy link
Collaborator Author

Removed the hover effect and also figured out why the toolbar flickered in like that: we need to have the on_layers_toggled callback method generate and set the layer_manager on the accessory_widget before we show the toolbar_footer. Otherwise, we end up showing the toolbar first for a momemnt since that's what's currently set in the toolbar_footer and then later the callback triggers and the content of the toolbar_footer are replaced by the layer_manager.

Copy link
Member

@giswqs giswqs left a comment

Choose a reason for hiding this comment

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

Awesome! It works as expected now. No more flickering.

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.

Thank you so much!

@naschmitz naschmitz merged commit 00b5bca into gee-community:master Jun 14, 2024
14 checks passed
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.

3 participants