QuantStack / jupytergis

JupyterGIS
BSD 3-Clause "New" or "Revised" License
15 stars 5 forks source link

Add a layers panel #17

Closed brichet closed 1 day ago

brichet commented 4 days ago

Add a layer panel to the left.

Peek 2024-06-26 12-01

Remaining for this PR:

Probably in a follow up PR:

github-actions[bot] commented 4 days ago

Binder :point_left: Launch a Binder on branch brichet/jupytergis/layers

brichet commented 2 days ago

My comments are only nitpicky comments about the naming of layersTree. It seems a bit heterogeneous in the code-base between layersTree, layerTree and treeLayers so I'm suggesting going for layersTree everywhere.

Your are right for some of them, but maybe not for all. I wanted to make the distinction between the tree itself (a IJGISLayerGroup object, what I called layerTree), and the list of layers coming from the tree (a (IJGISLayerGroup | string)[] object, that I called treeLayers).

The second come from the fact that I used a IJGISLayerGroup for the tree but we only need the list of layers, the other properties are useless. See https://github.com/QuantStack/jupytergis/issues/4#issuecomment-2182530646 for reference.

Maybe we should declare the tree as a (IJGISLayerGroup | string)[] to simplify. The only drawback is the duplication of the following schema, for the layersTree itself and the layers of a IJGISLayerGroup.

"items": {
  "oneOf": [
      {"type": "string"},
      {
        "type": "object",
        "$ref": "#/definitions/jGISLayerGroup"
      }
  ]
}

It would clarify the naming and the tree object, avoiding useless properties, since the tree should only be a list of layers and groups (if I'm not mistaken).

martinRenou commented 2 days ago

Oh ok I see.

Maybe we should declare the tree as a (IJGISLayerGroup | string)[] to simplify

That sounds good to me. Then you only operate on this layersTree (the root) which is of this type.

brichet commented 2 days ago

Oh ok I see.

Maybe we should declare the tree as a (IJGISLayerGroup | string)[] to simplify

That sounds good to me. Then you only operate on this layersTree (the root) which is of this type.

Exact, let's update it then.

brichet commented 1 day ago

@martinRenou I updated the schema