Skip to content

Feature/custom map improvements#8703

Draft
eliandoran wants to merge 3 commits intomainfrom
feature/custom_map_improvements
Draft

Feature/custom map improvements#8703
eliandoran wants to merge 3 commits intomainfrom
feature/custom_map_improvements

Conversation

@eliandoran
Copy link
Contributor

No description provided.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @eliandoran, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the custom map functionality by enabling users to define their own map layers via URLs and providing a new control to hide marker labels for better visual clarity. It also includes a refactoring of the map layer management system for increased flexibility and an improvement to the checkbox property component to handle inverted boolean states more intuitively.

Highlights

  • Custom Map Layers: Introduced the ability to use custom map layers by providing a URL, expanding beyond the previously supported predefined layers.
  • Hide Marker Labels: Added a new option to toggle the visibility of marker names (labels) on the map, allowing for cleaner map views, especially with many markers.
  • Refactored Map Layer Handling: The internal logic for managing map layers was refactored to use a more flexible layerData object instead of a simple layerName string, facilitating custom and built-in layer configurations.
  • Enhanced Checkbox Property: The CheckBoxProperty component now supports a reverseValue option, enabling checkboxes to represent inverted boolean logic (e.g., checked when the underlying label value is false) for improved user experience.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • apps/client/src/translations/en/translation.json
    • Added a new translation key, "show-labels", for the option to display marker names.
  • apps/client/src/widgets/collections/geomap/index.tsx
    • Imported MAP_LAYERS and MapLayer types for enhanced layer management.
    • Introduced a hideLabels state and a useLayerData hook to manage map layer configurations.
    • Modified the Map component to accept layerData instead of layerName for more flexible layer definitions.
    • Updated NoteWrapper, NoteMarker, and NoteGpxTrack components to pass and utilize the hideLabels prop, controlling marker label visibility.
    • Implemented the useLayerData hook to dynamically determine map layer data, supporting both custom URL-based layers and built-in options.
  • apps/client/src/widgets/collections/geomap/map.tsx
    • Updated the MapProps interface to use a layerData object of type MapLayer for map configuration.
    • Modified the Map component to accept and utilize the layerData prop for initializing map layers and determining the theme.
    • Removed direct lookup of MAP_LAYERS by layerName within the component, relying instead on the passed layerData.
  • apps/client/src/widgets/collections/geomap/map_layer.ts
    • Refactored the MapLayer interface into a union type to explicitly support both vector and raster layer configurations with common properties.
    • Updated the type definition for MAP_LAYERS to reflect the new MapLayer union type.
  • apps/client/src/widgets/note_bars/CollectionProperties.tsx
    • Modified the CheckBoxPropertyView component to correctly apply reverseValue logic when both setting and retrieving the checkbox state, ensuring intuitive behavior for inverted boolean properties.
  • apps/client/src/widgets/ribbon/collection-properties-config.tsx
    • Extended the CheckBoxProperty interface with a new reverseValue property to indicate if the checkbox state should be inverted.
    • Added a new checkbox property for "show marker names" (map:hideLabels) to the map configuration, with reverseValue set to true.
  • packages/commons/src/lib/attribute_names.ts
    • Added map:hideLabels to the Labels type definition, allowing it to be used as a note label attribute.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several improvements to the custom map feature. It adds the ability to show or hide marker names, supports custom raster map layers via URL, and includes a refactoring of how map layers are handled by passing a layerData object instead of just a layer name. The changes are well-structured, and the new reverseValue property on checkboxes is a nice UX improvement. I've found one issue related to incorrect dependencies in a useMemo hook that could lead to stale UI. Overall, great work on enhancing the map functionality.

color: note.getLabelValue("color") ?? "blue"
}
}), [ color, iconClass ]);
}), [ color, iconClass, hideLabels ]);
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The dependency array for useMemo is not correctly capturing all dependencies, which can lead to stale data for the track options.

  1. color and iconClass (from lines 282-283) are arrays returned by useNoteLabel ([value, setter]). Their references are stable across re-renders, so useMemo will not re-evaluate when the label values change. You should destructure the value from them (e.g., const [colorValue] = useNoteLabel(...)) and use colorValue in the dependency array.
  2. note.title is used on line 287 to build the startIcon, but it's not listed as a dependency. To make it reactive to changes, you should use const title = useNoteProperty(note, "title"); and include title in the dependency array.
  3. note.getLabelValue("color") is used for polyline_options on line 294. The color value from useNoteLabel should be used here as well to ensure consistency and reactivity.

A full fix would involve changes outside of this line, but the dependency array should be updated to include all reactive values it depends on.

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.

1 participant