Add CustomControl components #19
Conversation
Deploying svelte-maplibre-gl with
|
| Latest commit: |
a870266
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cd5cc156.svelte-maplibre-gl.pages.dev |
| Branch Preview URL: | https://feat-custom-control.svelte-maplibre-gl.pages.dev |
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Warning Rate limit exceeded@ciscorn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 using PR comments)
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: 5
🧹 Outside diff range and nitpick comments (9)
src/lib/maplibre/controls/CustomControl/Group.svelte (2)
6-8: Consider adding error handling for child rendering.While the current implementation is functional, it could benefit from error boundaries to gracefully handle rendering failures of child components.
Consider wrapping the render in a try-catch or implementing an error boundary:
<div class="maplibregl-ctrl-group"> - {@render children?.()} + {#try} + {@render children?.()} + {:catch error} + <div class="maplibregl-ctrl-group-error"> + <!-- Consider logging the error or showing a fallback UI --> + </div> + {/try} </div>
1-8: Well-structured building block for custom controls.This component follows good architectural principles:
- Single responsibility: focuses solely on grouping controls
- Composability: can be used with any custom control content
- Framework alignment: follows MapLibre's control patterns
Consider documenting usage examples in the component's JSDoc comments to help other developers understand its purpose and integration patterns.
src/lib/maplibre/controls/CustomControl/Icon.svelte (1)
1-9: Consider enhancing type safety and documentationThe props implementation could benefit from some improvements:
- Extract the props interface for reusability
- Add more specific types for event handlers
- Include JSDoc documentation for props
Consider applying these changes:
<script lang="ts"> import type { Snippet } from 'svelte'; + + /** Props interface for Icon component */ + interface IconProps { + /** Optional content to render inside the button */ + children?: Snippet; + /** Required CSS class name for additional styling */ + className: string; + /** Optional click event handler */ + onclick?: (event: MouseEvent) => void; + /** Optional context menu event handler */ + oncontextmenu?: (event: MouseEvent) => void; + } + let { children, onclick, oncontextmenu, className - }: { children?: Snippet; className: string; onclick?: () => void; oncontextmenu?: () => void } = $props(); + }: IconProps = $props(); </script>src/lib/maplibre/controls/CustomControl/Control.svelte (2)
14-20: Consider enhancing error messages for better debugging.The initialization and validation logic is solid, but the error messages could be more descriptive.
Consider this enhancement:
-if (!mapCtx.map) throw new Error('Map instance is not initialized.'); +if (!mapCtx.map) throw new Error('MapLibre GL map instance is not initialized. Ensure the Control component is used within a Map component.'); -if (!control && !children) throw new Error('You must provide either control or children.'); +if (!control && !children) throw new Error('CustomControl requires either a control object (IControl) or children elements. Please provide at least one of these props.');
47-49: Consider adding ARIA attributes for accessibility.The template structure is good, but could benefit from accessibility improvements.
Consider this enhancement:
-<div bind:this={el} class="maplibregl-ctrl"> +<div bind:this={el} class="maplibregl-ctrl" role="group" aria-label="Map custom control"> {@render children?.()} </div>src/content/examples/custom-control/MyControl.ts (2)
6-14: Consider adding readonly modifiers to private fields.Since these fields are only set in the constructor and never modified afterwards, they should be marked as readonly for better type safety.
class MyControl implements maplibregl.IControl { - private _container: HTMLElement | undefined; - private _toggleTerrain: () => boolean; - private _toggleHillshade: () => boolean; + private readonly _container: HTMLElement | undefined; + private readonly _toggleTerrain: () => boolean; + private readonly _toggleHillshade: () => boolean;
49-53: Simplify container removal with optional chaining.The current null check can be simplified using optional chaining.
onRemove() { - if (this._container && this._container.parentNode) { - this._container.parentNode.removeChild(this._container); - } + this._container?.parentNode?.removeChild(this._container); }🧰 Tools
🪛 Biome
[error] 50-50: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/content/examples/custom-control/CustomControl.svelte (1)
28-35: Consider making map configuration more flexibleThe map configuration has hardcoded values for zoom, pitch, center, etc. Consider:
- Making these configurable through props
- Adding validation for the maxPitch value
<MapLibre class="h-[60vh] min-h-[300px]" style={mapStyle} - zoom={12} - pitch={40} - maxPitch={85} - center={{ lng: 11.09085, lat: 47.3 }} + zoom={initialZoom} + pitch={initialPitch} + maxPitch={Math.min(maxPitch, 85)} + center={initialCenter} >src/lib/maplibre/index.ts (1)
46-46: Consider using .ts extension for TypeScript importsThe export follows the established pattern and is well-placed in the controls section. However, since this is a TypeScript file, consider using the .ts extension in the import path for consistency.
-export { default as CustomControl } from './controls/CustomControl/index.js'; +export { default as CustomControl } from './controls/CustomControl/index.ts';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
src/content/examples/Index.svelte(1 hunks)src/content/examples/custom-control/CustomControl.svelte(1 hunks)src/content/examples/custom-control/IconMoon.svelte(1 hunks)src/content/examples/custom-control/IconSun.svelte(1 hunks)src/content/examples/custom-control/MyControl.ts(1 hunks)src/content/examples/custom-control/content.svelte.md(1 hunks)src/lib/maplibre/controls/CustomControl/Control.svelte(1 hunks)src/lib/maplibre/controls/CustomControl/Group.svelte(1 hunks)src/lib/maplibre/controls/CustomControl/Icon.svelte(1 hunks)src/lib/maplibre/controls/CustomControl/index.ts(1 hunks)src/lib/maplibre/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/content/examples/Index.svelte
- src/content/examples/custom-control/IconMoon.svelte
- src/content/examples/custom-control/IconSun.svelte
- src/content/examples/custom-control/content.svelte.md
- src/lib/maplibre/controls/CustomControl/index.ts
🧰 Additional context used
🪛 Biome
src/content/examples/custom-control/MyControl.ts
[error] 50-50: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 eslint
src/lib/maplibre/controls/CustomControl/Control.svelte
[error] 24-24: '_' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (11)
src/lib/maplibre/controls/CustomControl/Group.svelte (2)
1-4: LGTM! Modern Svelte 5 syntax with proper typing.
The component correctly uses TypeScript with proper typing for dynamic content rendering. The use of Svelte 5's new $props() syntax is forward-looking.
6-8: LGTM! Follows MapLibre control conventions.
The component correctly uses MapLibre's control group class and properly handles optional children rendering.
src/lib/maplibre/controls/CustomControl/Icon.svelte (1)
1-20: Verify MapLibre control integration patterns
Let's ensure this component follows MapLibre's control implementation patterns and styling conventions.
✅ Verification successful
MapLibre control integration is correctly implemented
The Icon component follows MapLibre's control patterns correctly:
- It's part of a well-structured CustomControl implementation that properly implements MapLibre's IControl interface
- Uses the standard
maplibregl-ctrl-iconclass as expected - Works as part of a composable control system with Control/Group/Icon components
- The example usage shows correct integration with other MapLibre controls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other MapLibre control implementations for consistency
rg -t svelte "maplibregl-ctrl-icon" --glob '!src/lib/maplibre/controls/CustomControl/Icon.svelte'
# Check for any existing control button implementations
ast-grep --pattern 'class:maplibregl-ctrl-icon={$_}'
Length of output: 183
Script:
#!/bin/bash
# Search for other MapLibre control implementations without file type restriction
rg "maplibregl-ctrl-icon" --glob '!src/lib/maplibre/controls/CustomControl/Icon.svelte'
# Look for any MapLibre control related files
fd "control" --glob "*.svelte"
# Check for any MapLibre style related files or documentation
fd -e css -e scss | xargs rg "maplibregl-ctrl"
# Look for any MapLibre initialization or control setup
rg "new maplibregl.Control" -A 5
Length of output: 296
Script:
#!/bin/bash
# Look for MapLibre control related files
fd -t f "control" -E "Icon.svelte"
# Check the CustomControl directory structure and files
ls -R src/lib/maplibre/controls/
# Look for any MapLibre style imports or CSS files
fd -t f -e css -e scss
# Check the parent component that might be using this Icon
cat src/lib/maplibre/controls/CustomControl/CustomControl.svelte 2>/dev/null || true
Length of output: 1085
Script:
#!/bin/bash
# Check the Control.svelte implementation which is likely the parent component
cat src/lib/maplibre/controls/CustomControl/Control.svelte
# Check NavigationControl implementation for reference patterns
cat src/lib/maplibre/controls/NavigationControl.svelte
# Check the CustomControl example implementation
cat src/content/examples/custom-control/CustomControl.svelte
# Look for any MapLibre control type definitions or interfaces
rg "interface.*Control" -A 5
Length of output: 8456
src/lib/maplibre/controls/CustomControl/Control.svelte (2)
1-13: LGTM! Well-structured imports and type definitions.
The Props interface is well-defined with proper TypeScript types, and the imports are appropriate for the functionality needed.
33-44: Verify control cleanup behavior across different scenarios.
The lifecycle management looks good, with proper cleanup in both the effect and onDestroy hooks.
Let's verify the cleanup behavior:
✅ Verification successful
Control cleanup behavior is consistently implemented across the codebase
The verification shows that all control components in the codebase follow the same robust cleanup pattern:
- Removing controls in the
$effectbefore adding new ones to prevent duplicates - Properly cleaning up controls in
onDestroyhooks
This is consistently implemented across all control components:
- CustomControl/Control.svelte
- GlobeControl.svelte
- TerrainControl.svelte
- NavigationControl.svelte
- FullScreenControl.svelte
- LogoControl.svelte
- AttributionControl.svelte
- ScaleControl.svelte
- GeolocateControl.svelte
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other control implementations to ensure consistent cleanup patterns
ast-grep --pattern 'onDestroy(() => {
$$$
map?.removeControl($$$)
$$$
})'
Length of output: 81
Script:
#!/bin/bash
# Let's try a broader search for control cleanup patterns
# First, let's find all files that might contain control-related code
fd -e svelte -e ts -e js | xargs rg -l "removeControl|addControl"
# Then, let's look for cleanup patterns in these files
rg "onDestroy|cleanup|dispose" -A 5 -B 5
Length of output: 29448
src/content/examples/custom-control/MyControl.ts (2)
1-4: LGTM! Clear and well-defined interface.
The interface is well-structured with clear method signatures for toggle functionality.
56-56: LGTM! Clear and explicit export.
The named export is appropriate for this module.
src/content/examples/custom-control/CustomControl.svelte (3)
16-25: LGTM! Clean control initialization with proper callbacks
The control initialization follows best practices with atomic state updates and proper return values.
7-14: Verify map style URLs availability and accessibility
The map style URLs are hardcoded to use CartoDB's basemaps. Consider:
- Adding error handling for failed style loading
- Making the URLs configurable through props
✅ Verification successful
Map style URLs are accessible and properly configured
Both CartoDB basemap style URLs are accessible and return valid responses with proper CORS headers (Access-Control-Allow-Origin: *) and content type (application/json). The URLs are being served through a reliable CDN with good caching, indicating they are production-ready endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the map style URLs are accessible
curl -I "https://basemaps.cartocdn.com/gl/dark-matter-gl-style/style.json"
curl -I "https://basemaps.cartocdn.com/gl/voyager-gl-style/style.json"
Length of output: 2145
68-73: Review conditional rendering performance impact
The conditional rendering of Terrain and HillshadeLayer components might cause performance issues when frequently toggled. Consider:
- Using opacity instead of removing/adding components
- Adding transition animations for smoother toggling
src/lib/maplibre/index.ts (1)
46-46: Verify the CustomControl component implementation
Let's ensure the referenced CustomControl component exists and is properly implemented.
✅ Verification successful
Let me gather more information about the CustomControl implementation.
Let me check the implementation of the Svelte components that make up the CustomControl.
CustomControl component is properly implemented and follows MapLibre control patterns
The CustomControl is a well-structured Svelte component that:
- Implements the required MapLibre IControl interface
- Provides three subcomponents (Control, Group, Icon) for flexible control creation
- Properly handles lifecycle with map control addition/removal
- Includes proper TypeScript typing and error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of CustomControl component
# Check if the file exists
fd "CustomControl" "src/lib/maplibre/controls"
# Check the implementation
ast-grep --pattern 'export default class CustomControl {
$$$
}'
Length of output: 153
Script:
#!/bin/bash
# Check the content of the index file in CustomControl directory
cat src/lib/maplibre/controls/CustomControl/index.ts
# Also check for any test files to understand the expected behavior
fd "CustomControl" "src" -e test.ts -e spec.ts
Length of output: 288
Script:
#!/bin/bash
# Check the implementation of the Control, Group, and Icon components
cat src/lib/maplibre/controls/CustomControl/Control.svelte
cat src/lib/maplibre/controls/CustomControl/Group.svelte
cat src/lib/maplibre/controls/CustomControl/Icon.svelte
Length of output: 1940
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/content/examples/custom-control/CustomControl.svelte (1)
10-14: Consider making map style URLs configurableThe map style URLs are currently hardcoded. Consider making them configurable through component props to improve reusability and maintainability.
+export let darkModeStyle = 'https://basemaps.cartocdn.com/gl/dark-matter-gl-style/style.json'; +export let lightModeStyle = 'https://basemaps.cartocdn.com/gl/voyager-gl-style/style.json'; const mapStyle = $derived( isDarkMode - ? 'https://basemaps.cartocdn.com/gl/dark-matter-gl-style/style.json' - : 'https://basemaps.cartocdn.com/gl/voyager-gl-style/style.json' + ? darkModeStyle + : lightModeStyle );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/content/examples/custom-control/CustomControl.svelte(1 hunks)src/lib/maplibre/controls/CustomControl/Icon.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/maplibre/controls/CustomControl/Icon.svelte
🔇 Additional comments (4)
src/content/examples/custom-control/CustomControl.svelte (4)
5-5: Update import path from .js to .ts
The import statement needs to be updated to match the correct file extension.
16-25: LGTM! Clean implementation of control callbacks
The control initialization is well-structured with clear toggle functions that maintain single responsibility.
36-59: LGTM! Well-organized control layout
The control layout is well-structured with clear positioning and good use of CustomControl components. The implementation allows for both IControl injection and custom Svelte elements.
61-67: Consider error handling for terrain tiles
The terrain tiles configuration could benefit from improved error handling and configuration.
|
@ciscorn Thank you for comments! I revised as you advised. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
src/content/examples/animate-images/AnimateImages.svelte (2)
7-13: Consider adding performance optimizations and user controls.The current implementation continuously updates frames regardless of tab visibility or user preference.
Consider these improvements:
- Add visibility detection:
$effect(() => { let animationFrameId: number; + const isVisible = document.visibilityState === 'visible'; + + function handleVisibilityChange() { + if (document.visibilityState === 'hidden') { + cancelAnimationFrame(animationFrameId); + } else { + animationFrameId = requestAnimationFrame(update); + } + } + + document.addEventListener('visibilitychange', handleVisibilityChange); function update() { frame = Math.round((performance.now() / 1000) * 5) % FRAME_COUNT; animationFrameId = requestAnimationFrame(update); } - animationFrameId = requestAnimationFrame(update); + if (isVisible) { + animationFrameId = requestAnimationFrame(update); + } return () => { cancelAnimationFrame(animationFrameId); + document.removeEventListener('visibilitychange', handleVisibilityChange); }; });
- Add user controls:
+let isPlaying = $state(true); +let speed = $state(5); + $effect(() => { let animationFrameId: number; + if (!isPlaying) return; + function update() { - frame = Math.round((performance.now() / 1000) * 5) % FRAME_COUNT; + frame = Math.round((performance.now() / 1000) * speed) % FRAME_COUNT; animationFrameId = requestAnimationFrame(update); } animationFrameId = requestAnimationFrame(update); return () => { cancelAnimationFrame(animationFrameId); }; });
7-13: Consider adding error boundaries for the animation loop.The animation loop could potentially throw errors if the performance API is not available or if frame calculations fail.
Add error handling:
$effect(() => { let animationFrameId: number; function update() { + try { frame = Math.round((performance.now() / 1000) * 5) % FRAME_COUNT; animationFrameId = requestAnimationFrame(update); + } catch (error) { + console.error('Animation update failed:', error); + cancelAnimationFrame(animationFrameId); + } } animationFrameId = requestAnimationFrame(update); return () => { cancelAnimationFrame(animationFrameId); }; });src/lib/maplibre/controls/CustomControl.svelte (3)
15-16: Enhance props validation and error messaging.While the basic validation is present, consider making it more robust and user-friendly.
Consider this improvement:
-if (!givenControl && !children) throw new Error('You must provide either control or children.'); +if (!givenControl && !children) { + throw new Error( + 'CustomControl requires either a control prop (IControl instance) or children prop (Svelte components). ' + + 'Please provide one of these props to render the control.' + ); +}
18-19: Improve error message for map initialization.The current error message could be more helpful for debugging.
Consider this improvement:
-if (!mapCtx.map) throw new Error('Map instance is not initialized.'); +if (!mapCtx.map) { + throw new Error( + 'Map instance is not initialized. Ensure CustomControl is used within a Map component context.' + ); +}
1-52: Consider adding JSDoc documentation for better developer experience.The component is well-architected, but adding comprehensive documentation would improve its usability.
Consider adding JSDoc comments at the component level:
/** * CustomControl component for MapLibre GL JS. * Allows integration of both pre-defined MapLibre controls and custom Svelte components. * * @component * @example * ```svelte * <CustomControl position="top-right"> * <button>Custom Button</button> * </CustomControl> * ``` * * @example * ```svelte * <CustomControl position="top-left" control={new NavigationControl()} /> * ``` */🧰 Tools
🪛 eslint
[error] 43-43: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
src/content/examples/custom-control/MyControl.ts (2)
1-4: Add JSDoc documentation to interface methods.Consider adding documentation to explain the purpose and behavior of each toggle method.
interface MyControlConstructorOptions { + /** Toggles the terrain layer. Returns true if terrain is enabled after toggle. */ toggleTerrain: () => boolean; + /** Toggles the hillshade layer. Returns true if hillshade is enabled after toggle. */ toggleHillshade: () => boolean; }
6-14: Add parameter validation in constructor.Consider adding checks for required callbacks to prevent runtime errors.
constructor(options: MyControlConstructorOptions) { + if (!options?.toggleTerrain || !options?.toggleHillshade) { + throw new Error('Both toggleTerrain and toggleHillshade callbacks are required'); + } this._toggleTerrain = options.toggleTerrain; this._toggleHillshade = options.toggleHillshade; }src/content/examples/limit-interaction/LimitInteraction.svelte (3)
8-17: Add TypeScript type definitions for state variables.Consider adding explicit type definitions for better type safety and code maintainability.
-let zoomRange = $state([5, 11]); -let pitchRange = $state([0, 60]); -let boxZoom = $state(true); +let zoomRange = $state<[number, number]>([5, 11]); +let pitchRange = $state<[number, number]>([0, 60]); +let boxZoom = $state<boolean>(true);
23-24: Consider adjusting initial zoom for better UX.The initial zoom (5) matches the lower bound of zoomRange [5, 11]. If a user decreases the minimum zoom below 5, the map won't automatically adjust to show this new range. Consider setting the initial zoom to a value well within the range.
- zoom={5} + zoom={8}Also applies to: 33-34
38-39: Enhance control panel accessibility and usability.Consider the following improvements:
- Add
aria-labelto the control panel for better accessibility- Increase the background opacity for better readability on dark maps
- Group related controls (e.g., zoom/pitch controls, touch controls) using fieldset or similar
- <div class="absolute right-3 top-3 z-10 flex flex-col gap-1 rounded bg-background/60 p-3 text-sm backdrop-blur"> + <div + class="absolute right-3 top-3 z-10 flex flex-col gap-1 rounded bg-background/80 p-3 text-sm backdrop-blur" + aria-label="Map interaction controls" + role="region" + >src/content/examples/custom-control/CustomControl.svelte (1)
1-88: Consider externalizing configurationThe component has several hardcoded values that could be made configurable:
- Map style URLs
- Initial center coordinates
- Terrain tiles URL
- Zoom levels
Consider extracting these into a configuration object that can be passed as props.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
README.md(0 hunks)package.json(1 hunks)src/content/examples/Index.svelte(1 hunks)src/content/examples/animate-images/AnimateImages.svelte(1 hunks)src/content/examples/custom-control/CustomControl.svelte(1 hunks)src/content/examples/custom-control/MyControl.ts(1 hunks)src/content/examples/custom-control/content.svelte.md(1 hunks)src/content/examples/hover-styles/HoverStyles.svelte(0 hunks)src/content/examples/limit-interaction/LimitInteraction.svelte(1 hunks)src/content/examples/limit-interaction/content.svelte.md(1 hunks)src/lib/maplibre/controls/CustomControl.svelte(1 hunks)src/lib/maplibre/index.ts(1 hunks)src/lib/maplibre/map/MapLibre.svelte(3 hunks)src/routes/components/+page.ts(1 hunks)src/routes/examples/+page.ts(1 hunks)
💤 Files with no reviewable changes (2)
- README.md
- src/content/examples/hover-styles/HoverStyles.svelte
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/content/examples/limit-interaction/content.svelte.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/content/examples/Index.svelte
- src/content/examples/custom-control/content.svelte.md
- src/lib/maplibre/index.ts
🧰 Additional context used
🪛 Biome
src/content/examples/custom-control/MyControl.ts
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 eslint
src/lib/maplibre/controls/CustomControl.svelte
[error] 43-43: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
🔇 Additional comments (9)
src/routes/examples/+page.ts (1)
4-4: 🛠️ Refactor suggestion
Consider using more specific redirect status codes.
The change from 303 to 302 might lead to inconsistent behavior across browsers. Consider using:
- 303 (See Other) if you want to force a GET request
- 307 (Temporary Redirect) if you want to preserve the original HTTP method
The 302 status code has historically led to inconsistent handling of the HTTP method across different browsers.
Let's verify the redirect behavior across the application:
src/routes/components/+page.ts (1)
4-4: Consider using 303 instead of 302 for better browser consistency.
While 302 works for this static redirect, 303 is generally preferred in modern web applications as it ensures consistent GET requests across all browsers. However, if this is intentionally aligned with the change in src/routes/examples/+page.ts, the current implementation is acceptable.
Let's verify if this route handles any form submissions that might be affected by this change:
✅ Verification successful
Let me gather more information about the form handling by using different file patterns:
Let me check the content and purpose of the markdown component this redirects to:
The 302 redirect is appropriate for this use case
This is a simple static redirect from /components to /components/markdown without any form handling or POST operations. The change from 303 to 302 is safe in this context as:
- It's a GET-to-GET redirect (no form submissions involved)
- The redirect is temporary and straightforward
- There are no form handlers or POST operations in this route that could be affected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for form handling in the components route
# Look for form submissions or POST handlers that might be affected by the redirect
# Search for form elements or POST handlers in related component files
rg -t svelte 'method=(["'\''](POST|post)["'\'']|POST|post)' src/routes/components/
rg -t svelte '<form' -A 5 src/routes/components/
# Check for server-side POST handlers
rg -t ts 'POST|post' src/routes/components/
Length of output: 503
Script:
#!/bin/bash
# Search for form-related patterns without file type restrictions
rg '<form' src/routes/components/
rg 'method=' src/routes/components/
# Check the actual component file being redirected to
rg -l 'markdown' src/routes/components/
Length of output: 142
Script:
#!/bin/bash
# Check the markdown component and its related files
cat src/routes/components/+page.ts
fd markdown src/routes/components/ --type f
Length of output: 192
src/lib/maplibre/controls/CustomControl.svelte (2)
1-14: LGTM! Well-structured type definitions and imports.
The Props interface is well-defined with appropriate types, and the documentation reference is helpful.
48-52: LGTM! Clean and efficient template implementation.
The template follows Svelte best practices with proper class handling and safe children rendering.
src/content/examples/custom-control/CustomControl.svelte (3)
10-10: Update the file extension in the import statement
The import statement uses .js extension but the file is actually a TypeScript file.
- import { MyControl } from './MyControl.js';
+ import { MyControl } from './MyControl.ts';23-32: LGTM: Control initialization is well implemented
The MyControl instance is properly initialized with callbacks that handle state updates and return the new state.
74-80: Add error handling for terrain tiles
The terrain tiles URL is hardcoded and lacks error handling.
Let's verify the terrain tiles URL availability:
#!/bin/bash
# Check if the terrain tiles endpoint is accessible
curl -I https://demotiles.maplibre.org/terrain-tiles/0/0/0.pngsrc/lib/maplibre/map/MapLibre.svelte (2)
544-548: LGTM! Clean and safe template structure.
The template changes improve component stability by:
- Ensuring children are only rendered after map initialization
- Using proper optional chaining for children rendering
- Double-checking both map existence and loaded state
429-432: LGTM! Verify initialization behavior.
The addition of the !firstRun check to maxPitch/minPitch updates is consistent with other map options and prevents potential race conditions during initialization.
Let's verify that this change doesn't affect the initial map state:
Also applies to: 441-444
✅ Verification successful
Verified: !firstRun check for pitch updates is correct and safe
The code analysis shows that pitch handling is well-structured:
- Initial pitch value is properly bound via
pitch = $bindable(undefined) - Pitch updates are handled in a comprehensive transform update block that includes other related properties
- The component properly sets up pitch-related event listeners (
pitch,pitchstart,pitchend)
The !firstRun check is safe because:
- The map's initial pitch is set through the map initialization options
- Post-initialization updates are handled through the transform update logic
- This matches the pattern used for other transform-related properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any initialization-related issues with pitch settings
# Look for patterns where pitch settings are crucial during map initialization
# Search for pitch-related initialization code
rg -A 5 'pitch.*init|init.*pitch'
# Search for any pitch-related issues or bugs reported
rg -A 5 'pitch.*bug|bug.*pitch|pitch.*issue|issue.*pitch'
# Look for test files related to pitch functionality
fd -e test.ts -e test.js -e spec.ts -e spec.js | xargs rg 'pitch'
Length of output: 161
Script:
#!/bin/bash
# Let's try a different approach to analyze pitch-related functionality
# Look for pitch-related code in MapLibre.svelte
rg -A 10 "pitch" "src/lib/maplibre/map/MapLibre.svelte"
# Search for map initialization code to understand the setup flow
ast-grep --pattern 'onMount($$$)'
# Look for any pitch-related props or options in the component
ast-grep --pattern 'export let $_ = $_'
Length of output: 2607
| $effect(() => { | ||
| if (control) { | ||
| mapCtx.map?.addControl(control, position); | ||
| } | ||
| return () => { | ||
| control && mapCtx.map?.removeControl(control); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Fix linting error and improve cleanup logic.
The effect hook has a linting error and could benefit from more explicit cleanup.
Apply this improvement:
$effect(() => {
if (control) {
mapCtx.map?.addControl(control, position);
}
return () => {
- control && mapCtx.map?.removeControl(control);
+ if (control && mapCtx.map) {
+ mapCtx.map.removeControl(control);
+ }
};
});📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $effect(() => { | |
| if (control) { | |
| mapCtx.map?.addControl(control, position); | |
| } | |
| return () => { | |
| control && mapCtx.map?.removeControl(control); | |
| }; | |
| }); | |
| $effect(() => { | |
| if (control) { | |
| mapCtx.map?.addControl(control, position); | |
| } | |
| return () => { | |
| if (control && mapCtx.map) { | |
| mapCtx.map.removeControl(control); | |
| } | |
| }; | |
| }); |
🧰 Tools
🪛 eslint
[error] 43-43: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
ciscorn
force-pushed
the
feat/custom-control
branch
from
3ece5ce to
a870266
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/lib/maplibre/controls/CustomControl.svelte (2)
15-16: Enhance error message for better debugging.While the validation is correct, the error message could be more descriptive to help developers understand the requirements better.
-if (!givenControl && !children) throw new Error('You must provide either control or children.'); +if (!givenControl && !children) throw new Error('CustomControl requires either a control prop (IControl instance) or children prop (Svelte component/snippet) to render.');
48-52: Consider adding ARIA attributes for accessibility.The control might benefit from appropriate ARIA attributes to improve accessibility.
-<div bind:this={el} class={`maplibregl-ctrl ${className}`} class:maplibregl-ctrl-group={group}> +<div + bind:this={el} + class={`maplibregl-ctrl ${className}`} + class:maplibregl-ctrl-group={group} + role="group" + aria-label="Map custom control"> {@render children?.()} </div>src/content/examples/custom-control/MyControl.ts (3)
1-4: Add JSDoc documentation for better maintainability.Consider adding JSDoc comments to document the interface and its methods, including parameter descriptions and return value meanings.
+/** + * Options for initializing MyControl + */ interface MyControlConstructorOptions { + /** Toggle terrain layer visibility. Returns true if terrain is enabled. */ toggleTerrain: () => boolean; + /** Toggle hillshade layer visibility. Returns true if hillshade is enabled. */ toggleHillshade: () => boolean; }
6-14: Improve type safety for container field.Consider using a more specific type for the container field to better handle its lifecycle.
class MyControl implements maplibregl.IControl { - private _container: HTMLElement | undefined; + private _container: HTMLDivElement | null = null; private _toggleTerrain: () => boolean; private _toggleHillshade: () => boolean;
16-49: Consider implementing keyboard navigation support.While previous comments covered styling and accessibility improvements, the control should also support keyboard navigation between buttons.
onAdd() { this._container = document.createElement('div'); - this._container.className = 'maplibregl-ctrl maplibregl-ctrl-group p-2 rounded flex w-[240px] gap-x-2'; + this._container.className = 'maplibregl-ctrl maplibregl-ctrl-group p-2 rounded flex w-[240px] gap-x-2'; + this._container.setAttribute('role', 'toolbar'); + this._container.setAttribute('aria-label', 'Map layer controls'); const toggleTerrain = document.createElement('button'); toggleTerrain.textContent = 'Disable Terrain'; toggleTerrain.type = 'button'; + toggleTerrain.tabIndex = 0; // ... rest of the button configuration ... const toggleHillshade = document.createElement('button'); toggleHillshade.textContent = 'Disable Hillshade'; toggleHillshade.type = 'button'; + toggleHillshade.tabIndex = 0; // ... rest of the button configuration ... + // Add keyboard navigation + this._container.addEventListener('keydown', (e) => { + if (e.key === 'ArrowRight' || e.key === 'ArrowLeft') { + const buttons = [toggleTerrain, toggleHillshade]; + const currentIdx = document.activeElement === toggleTerrain ? 0 : 1; + const nextIdx = e.key === 'ArrowRight' ? + (currentIdx + 1) % buttons.length : + (currentIdx - 1 + buttons.length) % buttons.length; + buttons[nextIdx].focus(); + } + });src/content/examples/custom-control/CustomControl.svelte (1)
35-35: Consider making map height more responsiveThe current height setting
h-[50vh] min-h-[200px]might not provide optimal responsiveness in all scenarios. Consider using CSS custom properties or Svelte props to make the height configurable.- <MapLibre class="h-[50vh] min-h-[200px]" style={mapStyle} zoom={12} pitch={40} maxPitch={85} bind:center> + <MapLibre + class="h-[var(--map-height,50vh)] min-h-[var(--map-min-height,200px)]" + style={mapStyle} + zoom={12} + pitch={40} + maxPitch={85} + bind:center + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
README.md(0 hunks)package.json(1 hunks)src/content/examples/Index.svelte(1 hunks)src/content/examples/custom-control/CustomControl.svelte(1 hunks)src/content/examples/custom-control/MyControl.ts(1 hunks)src/content/examples/custom-control/content.svelte.md(1 hunks)src/lib/maplibre/controls/CustomControl.svelte(1 hunks)src/lib/maplibre/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/content/examples/Index.svelte
- src/content/examples/custom-control/content.svelte.md
- src/lib/maplibre/index.ts
🧰 Additional context used
🪛 Biome
src/content/examples/custom-control/MyControl.ts
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 eslint
src/lib/maplibre/controls/CustomControl.svelte
[error] 43-43: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
🔇 Additional comments (10)
src/lib/maplibre/controls/CustomControl.svelte (4)
1-14: LGTM! Well-structured type definitions with helpful documentation link.
The Props interface is well-defined with appropriate types, and the documentation reference is valuable for maintainers.
21-36: Previous type safety concerns are still valid.
The concerns about type safety in the control implementation raised in the previous review still need to be addressed.
38-45: Previous effect hook concerns are still valid.
The linting error and cleanup logic improvements suggested in the previous review still need to be addressed.
🧰 Tools
🪛 eslint
[error] 43-43: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
1-52: Verify component exports and usage.
Let's ensure the component is properly exported and used consistently across the application.
✅ Verification successful
Component is properly exported and used correctly
The component is correctly exported in the library's index file and the usage examples demonstrate proper implementation with all supported props:
- Default export in
src/lib/maplibre/index.ts - Examples show correct usage of
position,control,class, and children props - Both control object and direct children approaches are demonstrated
- Component is being used in documentation examples
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the component is properly exported
rg -l "CustomControl" src/lib/maplibre/index.ts
# Find all usage instances to verify proper implementation
rg -l "CustomControl" src/content/examples/
Length of output: 228
Script:
#!/bin/bash
# Check how the component is exported in index.ts
rg "CustomControl" src/lib/maplibre/index.ts
# Check usage examples
rg -A 5 "CustomControl" src/content/examples/custom-control/content.svelte.md
rg -A 5 "CustomControl" src/content/examples/custom-control/CustomControl.svelte
Length of output: 1980
🧰 Tools
🪛 eslint
[error] 43-43: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
src/content/examples/custom-control/CustomControl.svelte (6)
10-10: Import extension needs to be updated
A previous review already identified this issue.
12-22: Well-structured state management
The state management is clean, properly typed, and follows Svelte best practices.
23-32: Clean control initialization with proper callback handling
The MyControl initialization is well-structured with clear toggle callbacks that properly manage state.
41-47: Accessibility improvements needed
A previous review already identified accessibility issues in these control buttons.
Also applies to: 52-63
70-70: Fix coordinate display
A previous review already identified the coordinate display bug.
74-80: Error handling needed for terrain tiles
A previous review already identified the need for error handling in terrain tiles.
There was a problem hiding this comment.
@Kanahiro Thank you!
I've made some simplifications:
- Removed
CustomControl.Groupas it can now be represented as<CustomControl group={true}> - Also removed
CustomControl.Groupsince it can be expressed with a standard HTML<button>.
Let me know if you have any thoughts or suggestions!
Close #6
What I did
CustomControlcomponents to add user-defined Control.Notes
None
Summary by CodeRabbit
Release Notes
New Features
Documentation
Version Update