Conversation
|
🚀 Deployed on https://690eba91808330fdc5e4691c--opengeos.netlify.app |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for layer ordering in MapLibre by implementing the before_id parameter, allowing layers to be inserted at specific positions in the layer stack rather than always on top.
- Adds Python methods to query style layers and find the first symbol layer
- Modifies
add_layer()to store and passbefore_idparameter to JavaScript - Updates JavaScript layer restoration and addition logic to respect
before_idpositioning
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| anymap/maplibre.py | Added style introspection methods and modified add_layer() to store before_id in metadata and pass it to JavaScript |
| anymap/static/maplibre_widget.js | Updated layer restoration to read beforeId from metadata and modified addLayer case to handle before_id parameter with fallback logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Dict: The style of the map. | ||
| """ | ||
| if isinstance(self._style, str): | ||
| response = requests.get(self.style, timeout=10) |
There was a problem hiding this comment.
Inconsistent variable reference: the code checks isinstance(self._style, str) but then calls requests.get(self.style, ...). Should use self._style in the requests.get call to match the isinstance check.
| response = requests.get(self.style, timeout=10) | |
| response = requests.get(self._style, timeout=10) |
| def get_style_layers(self, return_ids=False, sorted=True) -> List[str]: | ||
| """ | ||
| Get the names of the basemap layers. | ||
|
|
||
| Returns: | ||
| List[str]: The names of the basemap layers. |
There was a problem hiding this comment.
The return type annotation List[str] is incorrect. The method can return either List[str] (when return_ids=True), List[Dict] (when return_ids=False), or an empty list. The return type should be Union[List[str], List[Dict]] to accurately reflect the method's behavior.
| def get_style_layers(self, return_ids=False, sorted=True) -> List[str]: | |
| """ | |
| Get the names of the basemap layers. | |
| Returns: | |
| List[str]: The names of the basemap layers. | |
| def get_style_layers(self, return_ids=False, sorted=True) -> Union[List[str], List[Dict]]: | |
| """ | |
| Get the basemap layers or their IDs. | |
| Returns: | |
| Union[List[str], List[Dict]]: The list of basemap layer IDs (if return_ids=True), or the list of basemap layer dictionaries (if return_ids=False). |
| Get the names of the basemap layers. | ||
|
|
||
| Returns: | ||
| List[str]: The names of the basemap layers. |
There was a problem hiding this comment.
The docstring description is inaccurate. The method returns layer IDs (when return_ids=True) or full layer objects (when return_ids=False), not just 'names'. The description should clarify this and document the parameters return_ids and sorted.
| Get the names of the basemap layers. | |
| Returns: | |
| List[str]: The names of the basemap layers. | |
| Get the style layers from the map's style. | |
| Args: | |
| return_ids (bool): If True, return a list of layer IDs (str). If False, return a list of full layer objects (dict). | |
| sorted (bool): If True and return_ids is True, the list of IDs will be sorted alphabetically. | |
| Returns: | |
| List[str] or List[dict]: A list of layer IDs (if return_ids=True) or a list of full layer objects (if return_ids=False). |
| const currentLayers = model.get("_layers") || {}; | ||
| currentLayers[actualLayerId] = layerConfig; | ||
| model.set("_layers", currentLayers); | ||
| model.save_changes(); | ||
| attachFeaturePopup(actualLayerId); | ||
| } catch (error) { | ||
| console.error('[addLayer] Error adding layer:', error); | ||
| // Try adding without beforeId as fallback | ||
| if (beforeId) { | ||
| try { | ||
| map.addLayer(layerConfig); | ||
| const currentLayers = model.get("_layers") || {}; | ||
| currentLayers[actualLayerId] = layerConfig; | ||
| model.set("_layers", currentLayers); | ||
| model.save_changes(); | ||
| attachFeaturePopup(actualLayerId); |
There was a problem hiding this comment.
Duplicate code: lines 5703-5707 duplicate the logic from lines 5692-5696. Consider extracting this persistence and popup attachment logic into a helper function to reduce duplication.
| const currentLayers = model.get("_layers") || {}; | |
| currentLayers[actualLayerId] = layerConfig; | |
| model.set("_layers", currentLayers); | |
| model.save_changes(); | |
| attachFeaturePopup(actualLayerId); | |
| } catch (error) { | |
| console.error('[addLayer] Error adding layer:', error); | |
| // Try adding without beforeId as fallback | |
| if (beforeId) { | |
| try { | |
| map.addLayer(layerConfig); | |
| const currentLayers = model.get("_layers") || {}; | |
| currentLayers[actualLayerId] = layerConfig; | |
| model.set("_layers", currentLayers); | |
| model.save_changes(); | |
| attachFeaturePopup(actualLayerId); | |
| persistLayerAndAttachPopup(actualLayerId, layerConfig); | |
| } catch (error) { | |
| console.error('[addLayer] Error adding layer:', error); | |
| // Try adding without beforeId as fallback | |
| if (beforeId) { | |
| try { | |
| map.addLayer(layerConfig); | |
| persistLayerAndAttachPopup(actualLayerId, layerConfig); |
No description provided.