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

Implement LayerManager using LitElement + anywidget #2153

Merged
merged 20 commits into from
Nov 6, 2024

Conversation

naschmitz
Copy link
Collaborator

@naschmitz naschmitz commented Oct 10, 2024

Overview:

  • Used anywidget to implement the custom widget and LitElement for template rendering. LitElement because it's simple, lightweight, efficient, and safe.
  • Changed build system to hatchling. The old setuptools build system is insufficient for building custom widgets.
  • The diffs show 2000+ more lines being added to the codebase than deleted. This is expected. There are several large, one-time additions like package-lock.json, the pyproject.toml changes, and the style sheets to replicate legacy ipywidgets styles. Future widget migrations will have much smaller diffs.

Files and directories:

  • geemap/static/* contains the compiled ECMAScript modules. These files aren't manually written. Each widget must have it's own compiled ECMAScript module. A previous version of this pull request checked in static files, but there are security risks with that and the Python package installers should know how to build the TypeScript into modules.
  • js/* contains the TypeScript and CSS source files. These files are written manually.
  • js/ipywidgets_styles.ts implements the legacy styles with the non-custom ipywidgets.
  • js/material_styles.ts implements new material styles (only fonts right now).

How to build and test:

  • The Python side is unchanged.
  • To build the TypeScript source, run npm install and then npm run dev. This starts a development server that will watch the TypeScript and CSS source files for changes and re-build when necessary.

TODOs remaining:

  • Figure out how to simplify font loading. Currently, fonts are downloaded via an @import url command and then referenced in the stylesheets, but it involves duplication and brittle URLs. - @sufyanAbbasi
  • Remove setuptools references from pyproject.toml. - @naschmitz
  • Fix the build issues with this pull request. - @naschmitz
  • (Lower priority) Unit tests for the TypeScript code. - @sufyanAbbasi
  • (Improvement, Lower priority) Get jslink working in Colab. It'll make the UI feel so much more responsive. - @sufyanAbbasi
  • (Improvement, Lower priority) Add a delete confirmation dialog over the layer manager instead of inline with the rows.

@naschmitz naschmitz added enhancement improvements to geemap dependencies Pull requests that update a dependency file labels Oct 10, 2024
@naschmitz
Copy link
Collaborator Author

@giswqs Don't feel like you need to jump in and start reviewing this right away. I want to get feedback from other members of the Earth Engine team (@sufyanAbbasi in particular) and I'm sure there will be several iterations.

@giswqs
Copy link
Member

giswqs commented Oct 10, 2024

@naschmitz Sure, no problem. I will wait for other team members to review it first. I am not familar with TypeScript yet. Not much help I can provide yet.

Copy link
Collaborator

@sufyanAbbasi sufyanAbbasi left a comment

Choose a reason for hiding this comment

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

This is incredible work!! I am sending over some initial comments on the TS code, but I need to look at the Python code a little better.

js/layer_manager.ts Outdated Show resolved Hide resolved
js/utils.ts Show resolved Hide resolved
js/layer_manager_row.ts Outdated Show resolved Hide resolved
js/layer_manager_row.ts Outdated Show resolved Hide resolved
js/layer_manager.ts Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@sufyanAbbasi sufyanAbbasi left a comment

Choose a reason for hiding this comment

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

Thanks so much, this is really great! I'll send up a PR for testing as soon as I get them complete. I have one more nit down below, otherwise, looks good to me!

@naschmitz
Copy link
Collaborator Author

@giswqs - This pull request is ready for your review

@sufyanAbbasi
Copy link
Collaborator

Tests are done! https://github.com/naschmitz/geemap/tree/anywidget-test
I'll leave it to you to merge them in!

js/layer_manager.ts Outdated Show resolved Hide resolved
@giswqs
Copy link
Member

giswqs commented Oct 31, 2024

@naschmitz Sorry for the delay! I just started reviewing the PR. This is tremendous amount of work. Thank you for puttting this together.

Here are some feedback I would like to share.

  1. It seems that the basemap layer spinner continues to spin indefinitely, even after the basemap layer has finished loading. I'm not sure if you can replicate this issue. Here’s a screenshot for reference:
    image

  2. In previous versions, besides ticking the layer checkboxes, clicking directly on the layer names would also toggle the layers on and off. In this PR, it appears that layers can only be toggled by checking the boxes, and clicking the layer names has no effect.

@naschmitz
Copy link
Collaborator Author

@giswqs I think I've fixed both issues. Please take another look when you have time.

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.

Thank you very much for fixing it. I can confirm that now clicking on the layer name can toggle layer visibility.

One potential improvement would be to change the currsor shape to a pointer rather than text selection. when hoving on the layer name. Anyway, this is just a suggestion. No worries if it is difficult to change.

image

@naschmitz
Copy link
Collaborator Author

I fixed the cursor issue. It was as simple as adding cursor: pointer; to the relevant CSS class.

@naschmitz naschmitz merged commit 1d01bdc into gee-community:master Nov 6, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement improvements to geemap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants