Wire up polyline options for polygon and linestring using data-options #42
Conversation
WalkthroughThe updates revolve around refining geometry handling in a mapping application, focusing on streamlining node changes and options management within hyperleaflet objects. Functions have been fine-tuned to enhance data access and processing, while new features and tests bolster the versatility of creating and modifying leaflet geometries. The expanded testing coverage ensures the reliability and resilience of geometry operations. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Files skipped from review due to trivial changes (1)
Additional comments not posted (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/Geometry/leaflet-geometry.js (2 hunks)
- src/Geometry/test/geometry-object-handler.test.js (1 hunks)
- src/Geometry/test/hyperleaflet-geometry-handler.test.js (1 hunks)
Additional comments: 4
src/Geometry/test/geometry-object-handler.test.js (1)
- 82-86: The change to reformat the
coordinatesarray for theLineStringtype by splitting the coordinates into individual arrays for each point is correctly implemented. This adjustment ensures that the coordinates are structured in a way that is consistent with the expected format forLineStringgeometries in Leaflet. This change is crucial for the correct handling ofLineStringgeometries and aligns with the PR's objective to enhance geometry handling capabilities.src/Geometry/leaflet-geometry.js (2)
- 52-55: The addition of the
polylineOptionsparameter in thecreatePolygonGeometryfunction is correctly implemented. This change allows for more flexible and detailed configuration of polygons by enabling the specification of polyline options directly when creating a polygon. It's important to ensure that thepolylineOptionsare properly validated and sanitized before use to prevent potential security issues.- 109-121: The modification in the
createLeafletObjectfunction to include anoptionsfield in thedatasetparameter, which is parsed and passed along to the geometry creation function, is correctly implemented. This change supports the PR's objective by enabling the specification of polyline options for polygons directly through thedata-optionsattribute. It's crucial to ensure that theparsedOptionsare properly validated and sanitized to prevent potential security issues related to the parsing of user-supplied data.src/Geometry/test/hyperleaflet-geometry-handler.test.js (1)
- 124-139: The addition of a test case to verify the creation of a Leaflet polygon object with polyline options is correctly implemented. This test case effectively validates the new functionality introduced by the PR, ensuring that polyline options are correctly applied to the created polygon. It's important to ensure comprehensive coverage by including tests for various polyline options and edge cases to guarantee robustness.
andrewgryan
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/Geometry/leaflet-geometry.js (3 hunks)
- src/Geometry/test/hyperleaflet-geometry-handler.test.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/Geometry/leaflet-geometry.js
- src/Geometry/test/hyperleaflet-geometry-handler.test.js
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- src/Geometry/hyperleaflet-geometry-handler.js (2 hunks)
- src/Geometry/index.js (1 hunks)
- src/Geometry/leaflet-geometry.js (3 hunks)
- src/Geometry/test/hyperleaflet-geometry-handler.test.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/Geometry/leaflet-geometry.js
- src/Geometry/test/hyperleaflet-geometry-handler.test.js
Additional comments: 3
src/Geometry/index.js (1)
- 40-40: The addition of
'data-options'to theattributeFilterparameter is a positive change that aligns with the PR's objectives to enhance polygon configuration capabilities. Ensure that the rest of the system, particularly the components that consume theattributeFilterparameter, is updated to handle the new'data-options'attribute effectively.src/Geometry/hyperleaflet-geometry-handler.js (2)
- 30-30: The update to access
change['data-id']directly in thechangeNodeInHyperleafletfunction simplifies the code and makes it more readable. Ensure that all callers of this function are updated to pass the correct change object structure.- 53-58: The refactoring of
changeNodesInHyperleafletto handle an array of changes and a dataset, iterating over partial changes, is a thoughtful improvement. It's crucial to verify the integration and testing of these changes to ensure they work as expected within the broader system context.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/Geometry/hyperleaflet-geometry-handler.js (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Geometry/hyperleaflet-geometry-handler.js
|
This is ready for a review, some manual testing was performed to understand the shape of the data when updating geometry and options, add/remove and change events were triggered using a simple HTMX app. If I have time I'll add tests to cover them 😄 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/Geometry/leaflet-geometry.js (4 hunks)
Additional comments: 1
src/Geometry/leaflet-geometry.js (1)
- 119-132: The
createLeafletObjectfunction parses theoptionsfrom thedatasetand passes them to thecreateGeometryFnfunction. While this approach is generally correct, it's important to ensure that theoptionsJSON string is always well-formed and that there's a fallback or error handling mechanism in case of parsing errors. Additionally, the function could benefit from more explicit documentation or type checks to ensure that thedatasetparameter conforms to expected structures, especially since it directly influences geometry creation.
- Verify that the
optionsJSON string is always well-formed across all usages ofcreateLeafletObject.- Consider adding error handling for JSON parsing and documenting the expected structure of the
datasetparameter.
|
|
||
| const createLineGeometry = (parsedGeometry, options) => { | ||
| const { reverseOrderAll, reverseOrder } = options; | ||
| const { reverseOrderAll, reverseOrder, options: polylineOptions } = options; |
There was a problem hiding this comment.
The destructuring of options to extract polylineOptions is repeated in both createLineGeometry and createPolygonGeometry. This repetition could be reduced by refactoring the common logic into a utility function or by ensuring that the options object is structured appropriately before being passed to these functions.
Consider refactoring the common logic for handling options to improve code maintainability and reduce repetition.
src/Geometry/leaflet-geometry.js
Outdated
| console.log(change); | ||
| switch (change.attribute) { | ||
| case 'data-geometry': { | ||
| return changeGeometry(leafletObject, change); | ||
| } | ||
| case 'data-options': { | ||
| return changeOptions(leafletObject, change); | ||
| } |
There was a problem hiding this comment.
The changeLeafletObject function uses a console.log statement to log the change parameter. While logging can be useful for debugging, it's generally not advisable to leave such statements in production code, as it can clutter the console output and potentially leak sensitive information.
- Remove the
console.log(change);statement or replace it with a more controlled logging mechanism that can be enabled or disabled based on the environment.
- console.log(change);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| console.log(change); | |
| switch (change.attribute) { | |
| case 'data-geometry': { | |
| return changeGeometry(leafletObject, change); | |
| } | |
| case 'data-options': { | |
| return changeOptions(leafletObject, change); | |
| } | |
| switch (change.attribute) { | |
| case 'data-geometry': { | |
| return changeGeometry(leafletObject, change); | |
| } | |
| case 'data-options': { | |
| return changeOptions(leafletObject, change); | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/Geometry/leaflet-geometry.js (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Geometry/leaflet-geometry.js
|
@cemrehancavdar have you got time to review this? I drafted it during a recent hack session but I think it patches in some missing functionality in a reasonable system JS way. But because it was a hastily developed patch there may be design patterns that I've overlooked. |
|
Thank you for this PR! |
|
Also having a crazy busy month, will tidy this one up soon hopefully! |
andrewgryan
force-pushed
the
feature-style
branch
from
0cdca3d to
67890a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- src/Geometry/hyperleaflet-geometry-handler.js (2 hunks)
- src/Geometry/index.js (1 hunks)
- src/Geometry/leaflet-geometry.js (4 hunks)
- src/Geometry/test/geometry-object-handler.test.js (1 hunks)
- src/Geometry/test/hyperleaflet-geometry-handler.test.js (1 hunks)
Files skipped from review due to trivial changes (1)
- src/Geometry/test/geometry-object-handler.test.js
Additional comments not posted (4)
src/Geometry/index.js (1)
40-40: Ensure that downstream functions properly handle the 'data-options' attribute.Verification successful
The addition of 'data-options' to the
attributeFilterinsrc/Geometry/index.jsis supported by the codebase, with evidence of handling and testing for this attribute in bothsrc/Geometry/leaflet-geometry.jsandsrc/Geometry/test/hyperleaflet-geometry-handler.test.js. This suggests that downstream functions are prepared to properly handle the 'data-options' attribute.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'data-options' is correctly processed in downstream functions. rg --type js "data-options"Length of output: 254
src/Geometry/hyperleaflet-geometry-handler.js (2)
30-30: Verify that the new method of accessingrowIddirectly fromchangeis compatible with all expected change formats.Verification successful
The method of accessing
rowIddirectly fromchangeusingchange['data-id']is compatible with all expected change formats, as evidenced by the widespread use of 'data-id' across the project, including in test cases. This suggests that the change is consistent with the project's data handling practices and does not introduce compatibility issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new access method for `rowId` does not break existing functionality. rg --type js "data-id"Length of output: 12946
53-63: Ensure that the structured handling of changes inchangeNodesInHyperleafletis thoroughly tested with various change formats.src/Geometry/test/hyperleaflet-geometry-handler.test.js (1)
124-174: Ensure the new test cases for handling options in polygon and polyline objects are comprehensive and cover edge cases.Consider adding more tests to cover edge cases and ensure robustness of the new functionalities.
|
@cemrehancavdar I've added in some docs to cover the extension to allow |
andrewgryan
force-pushed
the
feature-style
branch
from
5bb2c9a to
b0dd7b0
Compare
|
| `#!css data-options` | ||
| : Specifies Polyline and Polygon options. See Leaflet documentation for [Polyline options](https://leafletjs.com/reference.html#polyline-option) and [Polygon options](https://leafletjs.com/reference.html#polygon). <br> | ||
| ```html | ||
| <span ... data-options='{"color": "red", "weight": 1, "opacity": 0.8}'></span> | ||
| ``` | ||
| Valid format: JSON representing options compatible with L.polyline and L.polygon leaflet methods. |
There was a problem hiding this comment.
Ensure consistency in the presentation of JSON data in documentation.
The JSON data in the data-options attribute example is not formatted consistently with other similar examples in the document. Consider formatting it with proper indentation to improve readability and maintain consistency across the documentation.
- <span ... data-options='{"color": "red", "weight": 1, "opacity": 0.8}'></span>
+ <span ... data-options='{ "color": "red", "weight": 1, "opacity": 0.8 }'></span>Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| `#!css data-options` | |
| : Specifies Polyline and Polygon options. See Leaflet documentation for [Polyline options](https://leafletjs.com/reference.html#polyline-option) and [Polygon options](https://leafletjs.com/reference.html#polygon). <br> | |
| ```html | |
| <span ... data-options='{"color": "red", "weight": 1, "opacity": 0.8}'></span> | |
| ``` | |
| Valid format: JSON representing options compatible with L.polyline and L.polygon leaflet methods. | |
| `#!css data-options` | |
| : Specifies Polyline and Polygon options. See Leaflet documentation for [Polyline options](https://leafletjs.com/reference.html#polyline-option) and [Polygon options](https://leafletjs.com/reference.html#polygon). <br> | |
| ```html | |
| <span ... data-options='{ "color": "red", "weight": 1, "opacity": 0.8 }'></span> |
Valid format: JSON representing options compatible with L.polyline and L.polygon leaflet methods.
</details>
<!-- suggestion_end -->
<!-- This is an auto-generated comment by CodeRabbit -->
| const { reverseOrderAll, reverseOrder, options: polylineOptions } = options; | ||
| const isLonLat = reverseOrderAll || reverseOrder !== undefined; | ||
| const geometry = isLonLat ? GeoJSON.coordsToLatLngs(parsedGeometry, 0) : parsedGeometry; | ||
| const leafletGeometry = polyline(geometry); | ||
| const leafletGeometry = polyline(geometry, polylineOptions); |
There was a problem hiding this comment.
Refactor the handling of polyline options to reduce code duplication between line and polygon geometries.
- const { reverseOrderAll, reverseOrder, options: polylineOptions } = options;
+ const { reverseOrderAll, reverseOrder, polylineOptions } = extractPolylineOptions(options);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const { reverseOrderAll, reverseOrder, options: polylineOptions } = options; | |
| const isLonLat = reverseOrderAll || reverseOrder !== undefined; | |
| const geometry = isLonLat ? GeoJSON.coordsToLatLngs(parsedGeometry, 0) : parsedGeometry; | |
| const leafletGeometry = polyline(geometry); | |
| const leafletGeometry = polyline(geometry, polylineOptions); | |
| const { reverseOrderAll, reverseOrder, polylineOptions } = extractPolylineOptions(options); | |
| const isLonLat = reverseOrderAll || reverseOrder !== undefined; | |
| const geometry = isLonLat ? GeoJSON.coordsToLatLngs(parsedGeometry, 0) : parsedGeometry; | |
| const leafletGeometry = polyline(geometry, polylineOptions); |
| function changeOptions(leafletObject, change) { | ||
| const { to: options } = change; | ||
| return leafletObject.setStyle(JSON.parse(options)); | ||
| } |
There was a problem hiding this comment.
Refine the implementation of changeOptions to include error handling for JSON parsing and ensure compatibility of options with setStyle.
function changeOptions(leafletObject, change) {
const { to: options } = change;
+ try {
+ const parsedOptions = JSON.parse(options);
+ return leafletObject.setStyle(parsedOptions);
+ } catch (error) {
+ console.error('Failed to parse options:', error);
+ return leafletObject; // Or handle the error as appropriate
+ }
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| function changeOptions(leafletObject, change) { | |
| const { to: options } = change; | |
| return leafletObject.setStyle(JSON.parse(options)); | |
| } | |
| function changeOptions(leafletObject, change) { | |
| const { to: options } = change; | |
| try { | |
| const parsedOptions = JSON.parse(options); | |
| return leafletObject.setStyle(parsedOptions); | |
| } catch (error) { | |
| console.error('Failed to parse options:', error); | |
| return leafletObject; // Or handle the error as appropriate | |
| } | |
| } |
|
It seems tracking coordinate and options change won't work right now. I'll dig in to it |
|
I may have broken the update handler to support swapping out a DOM element with the same At the moment I'm writing index.html files elsewhere on my file system to check the behaviour, which isn't ideal. |
2 participants
Add support for second positional argument to
L.polygonvia thedata-optionsattribute.Summary by CodeRabbit
usage.mdwith information on adding icons for Point geometries.