Add Marker's Label #157
Add Marker's Label #157
Conversation
WalkthroughVersion bumped to 2.3.0-SNAPSHOT in pom.xml. Introduced MarkerLabel class with label properties and JSON serialization. Added GoogleMapMarker.setLabel(MarkerLabel) to set marker labels. Updated AddMarkersDemo to support optional marker labels via new TextField and Checkbox, adjusted form reset, imports, layout, and license year. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/com/flowingcode/vaadin/addons/googlemaps/MarkerLabel.java (2)
1-1: Missing license header (inconsistent with project files)Other classes in this module include the Apache 2.0 license header. Please add it here for consistency and to satisfy license tooling.
Apply at the top of the file:
+/*- + * #%L + * Google Maps Addon + * %% + * Copyright (C) 2020 - 2024 Flowing Code + * %% + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * #L% + */
46-55: Javadoc formatting for className blockMinor style nit: lines don’t start with
*consistently. Consider reformatting for consistency with the rest of the file.Example:
- /** - The className property of the label's element (equivalent to the - element's class attribute). Multiple space-separated CSS classes can - be added. The font color, size, weight, and family can only be set via - the other properties of <code>MarkerLabel</code>. CSS classes should not - be used to change the position nor orientation of the label (e.g. using - translations and rotations) if also using <a - href="https://developers.google.com/maps/documentation/javascript/examples/marker-collision-management">marker - collision management</a>. */ + /** + * The className property of the label's element (equivalent to the + * element's class attribute). Multiple space-separated CSS classes can + * be added. The font color, size, weight, and family can only be set via + * the other properties of <code>MarkerLabel</code>. CSS classes should not + * be used to change the position nor orientation of the label (e.g. using + * translations and rotations) if also using + * <a href="https://developers.google.com/maps/documentation/javascript/examples/marker-collision-management"> + * marker collision management</a>. + */src/test/java/com/flowingcode/vaadin/addons/googlemaps/CustomizedMarkerIconsDemo.java (1)
51-59: Minor: indentation consistencyThe new lines are indented differently from surrounding code. For readability, align indentation with adjacent statements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/googlemaps/GoogleMapMarker.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/googlemaps/MarkerLabel.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/googlemaps/CustomizedMarkerIconsDemo.java(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/flowingcode/vaadin/addons/googlemaps/MarkerLabel.java (1)
src/test/resources/META-INF/resources/frontend/src/clustering-custom-renderer-example.js (1)
color(39-39)
🔇 Additional comments (2)
pom.xml (1)
7-7: Version bump OK; double-check semver policy for public API additionsYou’re adding a new public API (setLabel + MarkerLabel). If you follow semver, consider whether this should be a minor bump rather than a patch (2.2.x). If your policy allows new features in patch releases, this is fine—just confirm and update changelog/release notes accordingly.
src/test/java/com/flowingcode/vaadin/addons/googlemaps/CustomizedMarkerIconsDemo.java (1)
48-50: Anchor/labelOrigin sanity checkAnchor moved to (0,0) and label origin to (0,10). Assuming your marker image’s origin is top-left, this places the label 10px below the anchor. If the image origin differs (e.g., center), adjust accordingly.
src/main/java/com/flowingcode/vaadin/addons/googlemaps/GoogleMapMarker.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/googlemaps/MarkerLabel.java
Outdated
Show resolved
Hide resolved
|
Hi @AlexRoig. Thank you for your contribution, we’ll review it soon. |
|
A small issue I found with this that might require a deeper look is about styling. By default, the marker' labels are black. With a satellite map, the black text (or white) looks pretty bad and there's no contrast. For example, something like this: |
|
Hello @AlexRoig, I checked the issue with setting the className and it seems it will need more investigation. I will update the PR with some minor fixes and after the "marker's label" feature is merged I will create a new issue to properly handle the className problem. |
Add an option to display a marker label.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/flowingcode/vaadin/addons/googlemaps/MarkerLabel.java (2)
85-94: Prefer explicit guard and simple null checks over Optional side-effects in getJsonUsing
Optional.of(getText()).ifPresent(...)will NPE iftextis null, but the exception type/message will be opaque. Also, using Optional purely for side-effects is a readability smell. A straightforward approach improves clarity and keeps the fail-fast guarantee.Apply:
protected JsonObject getJson() { JsonObject js = Json.createObject(); - Optional.of(getText()).ifPresent(value -> js.put("text", value)); - Optional.ofNullable(getColor()).ifPresent(value -> js.put("color", value)); - Optional.ofNullable(getFontFamily()).ifPresent(value -> js.put("fontFamily", value)); - Optional.ofNullable(getFontSize()).ifPresent(value -> js.put("fontSize", value)); - Optional.ofNullable(getFontWeight()).ifPresent(value -> js.put("fontWeight", value)); - Optional.ofNullable(getClassName()).ifPresent(value -> js.put("className", value)); + js.put("text", Objects.requireNonNull(getText(), "MarkerLabel.text must not be null")); + if (getColor() != null) js.put("color", getColor()); + if (getFontFamily() != null) js.put("fontFamily", getFontFamily()); + if (getFontSize() != null) js.put("fontSize", getFontSize()); + if (getFontWeight() != null) js.put("fontWeight", getFontWeight()); + if (getClassName() != null) js.put("className", getClassName()); return js; }
68-73: Document theming constraints for className and suggest outline stylingGiven Google Maps renders label DOM outside the component’s shadow DOM, CSS classes referenced by
classNamemust be available globally. A short note in the Javadoc helps users succeed and addresses the contrast concern on satellite imagery.Apply:
/** * The className property of the label's element (equivalent to the element's class attribute). * Multiple space-separated CSS classes can be added. The font color, size, weight, and family can - * only be set via the other properties of <code>MarkerLabel</code>. + * only be set via the other properties of <code>MarkerLabel</code>. + * <p><b>Note:</b> The label element is created by Google Maps outside the add-on component's + * shadow DOM. Ensure any CSS classes used here are defined in a global stylesheet so they apply. + * For improved legibility on satellite maps, consider a white font with a dark outline via + * <code>text-shadow</code>. */src/test/java/com/flowingcode/vaadin/addons/googlemaps/AddMarkersDemo.java (2)
71-82: UX polish: focus label input when enabling “With Label”When users toggle “With Label”, moving focus to the text field reduces clicks. No behavior change otherwise.
// #if vaadin eq 0 withLabel.addValueChangeListener(event -> { labelText.setEnabled(event.getValue()); + if (event.getValue()) { + labelText.focus(); + } }); // #endif
122-127: Use className for outlined text; optionally auto-contrast based on map typeSetting
color = "white"is a good baseline for SATELLITE. Adding a CSS class with a dark outline greatly improves legibility across basemaps. If you want, you can also auto-pick a default text color from the current map type.- if(withLabel.getValue()) { - MarkerLabel label = new MarkerLabel(labelText.getValue()); - label.setColor("white"); - label.setFontWeight("bold"); - marker.setLabel(label); - } + if (withLabel.getValue()) { + String txt = labelText.getValue(); + if (txt != null && !txt.isBlank()) { + MarkerLabel label = new MarkerLabel(txt); + // Optional: choose a sensible default color + String defaultColor = + gmaps.getMapType() == MapType.SATELLITE ? "white" : "black"; + label.setColor(defaultColor); + label.setFontWeight("bold"); + // Add an outline/shadow class for contrast on any imagery + label.setClassName("gmaps-label-outline"); + marker.setLabel(label); + } else { + Notification.show("Please enter a label text"); + } + }Example global CSS to include in your app theme (or a global stylesheet):
.gmaps-label-outline { text-shadow: 2px 0 #000, -2px 0 #000, 0 2px #000, 0 -2px #000, 1px 1px #000, -1px -1px #000, 1px -1px #000, -1px 1px #000; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/googlemaps/GoogleMapMarker.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/googlemaps/MarkerLabel.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/googlemaps/AddMarkersDemo.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pom.xml
- src/main/java/com/flowingcode/vaadin/addons/googlemaps/GoogleMapMarker.java
🔇 Additional comments (2)
src/main/java/com/flowingcode/vaadin/addons/googlemaps/MarkerLabel.java (1)
44-46: Good null-safety on required text; earlier concern addressedAnnotating
textwith@NonNulland enforcing non-null in both constructors closes the hole wheregetJson()could emit{}. With Lombok, the generated setter will also null-check. Looks good.Also applies to: 75-83
src/test/java/com/flowingcode/vaadin/addons/googlemaps/AddMarkersDemo.java (1)
36-36: Demo wiring and layout look goodImport, control creation, and layout integration are straightforward and consistent with existing demo code.
Also applies to: 71-76, 145-146
src/test/java/com/flowingcode/vaadin/addons/googlemaps/AddMarkersDemo.java
Show resolved
Hide resolved
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
3 participants
Add an option to display a marker label.
Summary by CodeRabbit
New Features
Chores