Conversation
giswqs
commented
Nov 22, 2025
|
🚀 Deployed on https://69214ccbc9f444aad6a0e7ac--opengeos.netlify.app |
There was a problem hiding this comment.
Pull request overview
This PR adds a new add_legend method to the MapLibreMap class, enabling users to add customizable legends to their maps with support for built-in legend presets (NLCD and NWI) and various customization options including colors, shapes, and positioning.
- Adds
add_legendmethod with support for custom and built-in legend configurations - Implements three shape types (rectangle, circle, line) for legend items
- Includes two built-in legend presets for common land cover classifications
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print( | ||
| f"Warning: builtin_legend '{builtin_legend}' not found. Available: {list(BUILTIN_LEGENDS.keys())}" | ||
| ) | ||
| return | ||
| legend_dict = BUILTIN_LEGENDS[builtin_legend] | ||
|
|
||
| # Determine legend items | ||
| if legend_dict is not None: | ||
| labels = list(legend_dict.keys()) | ||
| colors = [legend_dict[label] for label in labels] | ||
| elif labels is not None and colors is not None: | ||
| if len(labels) != len(colors): | ||
| print("Error: labels and colors must have the same length") | ||
| return | ||
| else: | ||
| print( | ||
| "Error: Either legend_dict or both labels and colors must be provided" | ||
| ) | ||
| return |
There was a problem hiding this comment.
Error handling uses print statements instead of raising exceptions, which is inconsistent with the codebase patterns. Other methods in this file (e.g., add_popup at line 1303, add_widget_control at line 1370) raise ValueError for validation errors. This method should raise exceptions instead of printing and returning early.
Consider changing:
- Line 5249-5252: raise
ValueErrorinstead ofprint+return - Line 5261-5262: raise
ValueErrorinstead ofprint+return - Line 5264-5267: raise
ValueErrorinstead ofprint+return
| position (str, optional): The position of the legend on the map. Can be one of "top-left", | ||
| "top-right", "bottom-left", "bottom-right". Defaults to "bottom-right". |
There was a problem hiding this comment.
The position parameter documentation lists valid values but there's no validation to ensure only these values are accepted. Invalid position values will be passed directly to add_widget_control which may have its own validation. Consider adding validation at this level for better error messages and early failure detection:
if position not in ["top-left", "top-right", "bottom-left", "bottom-right"]:
raise ValueError("position must be one of 'top-left', 'top-right', 'bottom-left', or 'bottom-right'")| padding="8px", | ||
| border="2px solid grey", | ||
| border_radius="5px", | ||
| background_color=bg_color, |
There was a problem hiding this comment.
Potential XSS vulnerability: The bg_color parameter is inserted directly into the CSS without validation or sanitization. Similar to the color values in legend items, a malicious value could inject arbitrary CSS. Consider validating bg_color against expected patterns or escaping it appropriately.
| return | ||
|
|
||
| # Normalize colors (add # if not present) | ||
| colors = [f"#{c}" if not c.startswith("#") else c for c in colors] |
There was a problem hiding this comment.
Color normalization logic could fail if colors contain values that don't start with '#' but also aren't valid color codes (e.g., colors with "rgba()" or "rgb()" format). The current logic assumes all colors are either hex codes with or without '#'. Consider validating or handling other color formats, or document that only hex colors are supported.
| BUILTIN_LEGENDS = { | ||
| "NLCD": { | ||
| "11 Open Water": "466b9f", | ||
| "12 Perennial Ice/Snow": "d1def8", | ||
| "21 Developed, Open Space": "dec5c5", | ||
| "22 Developed, Low Intensity": "d99282", | ||
| "23 Developed, Medium Intensity": "eb0000", | ||
| "24 Developed High Intensity": "ab0000", | ||
| "31 Barren Land (Rock/Sand/Clay)": "b3ac9f", | ||
| "41 Deciduous Forest": "68ab5f", | ||
| "42 Evergreen Forest": "1c5f2c", | ||
| "43 Mixed Forest": "b5c58f", | ||
| "51 Dwarf Scrub": "af963c", | ||
| "52 Shrub/Scrub": "ccb879", | ||
| "71 Grassland/Herbaceous": "dfdfc2", | ||
| "72 Sedge/Herbaceous": "d1d182", | ||
| "73 Lichens": "a3cc51", | ||
| "74 Moss": "82ba9e", | ||
| "81 Pasture/Hay": "dcd939", | ||
| "82 Cultivated Crops": "ab6c28", | ||
| "90 Woody Wetlands": "b8d9eb", | ||
| "95 Emergent Herbaceous Wetlands": "6c9fb8", | ||
| }, | ||
| "NWI": { | ||
| "Freshwater Forested/Shrub Wetland": "#008837", | ||
| "Freshwater Emergent Wetland": "#7FC31C", | ||
| "Freshwater Pond": "#688CC0", | ||
| "Estuarine and Marine Wetland": "#66C2A5", | ||
| "Riverine": "#0190BF", | ||
| "Lake": "#13007C", | ||
| "Estuarine and Marine Deepwater": "#007C88", | ||
| "Other": "#B28656", | ||
| }, | ||
| } |
There was a problem hiding this comment.
The built-in legend dictionaries are defined as a local constant within the method. This makes them harder to discover, test, and reuse. Consider moving BUILTIN_LEGENDS to module or class level as a constant, similar to how other configuration constants are typically organized. This would also allow for easier extension and documentation of available built-in legends.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>