step in enabling strict typescript for geoportal #438
Conversation
There was a problem hiding this comment.
Pull Request Overview
Progress toward enabling strict TypeScript in geoportal by tightening types and simplifying null handling.
- Add explicit typing to helper and component functions, including L.Map and event types
- Change getPolygonPoints to return null when no polygon is present and update call sites accordingly
- Fix a state setter name typo and improve preview size computation flow
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/geoportal/src/app/helper/print.tsx | Type getPolygonPoints(map: L.Map), return null when no polygon, and adjust addPreviewWrapper usage to handle null. |
| apps/geoportal/src/app/components/map-print/PrintPreview.tsx | Type function params (map, events, flags), rename preview state setter, and update logic to handle nullable polygon points. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| export const getPolygonPoints = (map: L.Map) => { | ||
| const polygon = getPolygonByLeafletId(map); | ||
| if (polygon) { | ||
| const bounds = polygon.getBounds(); | ||
|
|
||
| const { _northEast, _southWest } = bounds; | ||
| const northEast = map.latLngToContainerPoint(_northEast); | ||
| const southWest = map.latLngToContainerPoint(_southWest); | ||
| const northWest = { | ||
| x: southWest.x, | ||
| y: northEast.y, | ||
| }; | ||
| const southEast = { | ||
| x: northEast.x, | ||
| y: southWest.y, | ||
| }; | ||
| if (!polygon) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
getPolygonPoints now returns null when no polygon is found, changing its contract from 'always object' to 'object | null'. Please make this explicit in the signature to improve clarity and catch misuses at compile time, e.g. add a named return type and annotate the function as returning that type or null.
| const target = e.originalEvent.target as HTMLElement; | ||
| const routedMap = target?.id === "routedMap"; | ||
| const glLayer = target?.classList.contains("leaflet-gl-layer"); |
There was a problem hiding this comment.
[nitpick] Leaflet click targets can be SVG elements (or other non-HTMLElement Elements). Prefer typing this as Element, or guard with an instanceof check, to avoid incorrect narrowing and to better match the DOM surface you're using (id, classList). For example: const target = e.originalEvent.target; if (!(target instanceof Element)) return; // then use target.id and target.classList.
| const target = e.originalEvent.target as HTMLElement; | |
| const routedMap = target?.id === "routedMap"; | |
| const glLayer = target?.classList.contains("leaflet-gl-layer"); | |
| const target = e.originalEvent.target; | |
| if (!(target instanceof Element)) { | |
| return; | |
| } | |
| const routedMap = target.id === "routedMap"; | |
| const glLayer = target.classList.contains("leaflet-gl-layer"); |
| if (polygon) { | ||
| const { northWest, northEast, southWest } = getPolygonPoints(map); | ||
| const wrapWidth = northEast.x - northWest.x; | ||
| const changePreviewSizes = (map: L.Map, orientation: string) => { |
There was a problem hiding this comment.
[nitpick] orientation is compared to 'portrait' and 'landscape'; narrow the parameter type to a string literal union ('portrait' | 'landscape') to improve type safety and reduce accidental misuse.
| const changePreviewSizes = (map: L.Map, orientation: string) => { | |
| const changePreviewSizes = (map: L.Map, orientation: 'portrait' | 'landscape') => { |
| const { _northEast, _southWest } = bounds; | ||
| const northEast = map.latLngToContainerPoint(_northEast); | ||
| const southWest = map.latLngToContainerPoint(_southWest); |
There was a problem hiding this comment.
[nitpick] Avoid relying on Leaflet's private LatLngBounds fields (_northEast/_southWest). Use the public API for forward compatibility: const northEastLatLng = bounds.getNorthEast(); const southWestLatLng = bounds.getSouthWest(); then pass those to latLngToContainerPoint.
| const { _northEast, _southWest } = bounds; | |
| const northEast = map.latLngToContainerPoint(_northEast); | |
| const southWest = map.latLngToContainerPoint(_southWest); | |
| const northEastLatLng = bounds.getNorthEast(); | |
| const southWestLatLng = bounds.getSouthWest(); | |
| const northEast = map.latLngToContainerPoint(northEastLatLng); | |
| const southWest = map.latLngToContainerPoint(southWestLatLng); |
|
@PavelOlkhovoi not using the private fields seems like a good idea. leaflet has methods for getting the corners |
test print module.