-
-
Notifications
You must be signed in to change notification settings - Fork 570
fix(print-layout): improve atlas feature framing #1783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
71aa3d9
dea711d
1a7b448
60cdc8a
91e72bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /** Temporary inverted-fill mask for the active Print Layout atlas feature. */ | ||
| import { buildInvertedMask } from "@geolibre/map"; | ||
| import type { Feature, FeatureCollection, MultiPolygon, Polygon } from "geojson"; | ||
| import type { GeoJSONSource, Map as MapLibreMap } from "maplibre-gl"; | ||
|
|
||
| const SOURCE_ID = "geolibre-print-atlas-mask"; | ||
| const FILL_LAYER_ID = "geolibre-print-atlas-mask-fill"; | ||
|
|
||
| /** Remove the atlas mask source and layer, if present. */ | ||
| export function clearAtlasFeatureMask(map: MapLibreMap): void { | ||
| if (map.getLayer(FILL_LAYER_ID)) map.removeLayer(FILL_LAYER_ID); | ||
| if (map.getSource(SOURCE_ID)) map.removeSource(SOURCE_ID); | ||
| } | ||
|
|
||
| /** | ||
| * Show a translucent inverted fill around one polygon atlas feature. | ||
| * | ||
| * @param map - MapLibre map used by the Print Layout capture. | ||
| * @param feature - Current coverage feature. | ||
| * @returns Whether a polygon mask could be rendered. | ||
| */ | ||
| export function showAtlasFeatureMask(map: MapLibreMap, feature: Feature | undefined): boolean { | ||
| if (feature?.geometry?.type !== "Polygon" && feature?.geometry?.type !== "MultiPolygon") { | ||
| clearAtlasFeatureMask(map); | ||
| return false; | ||
| } | ||
| const collection: FeatureCollection<Polygon | MultiPolygon> = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor efficiency nit: Confidence: low — likely a negligible cost for typical polygon complexity, but could matter for large/complex coverage geometries exported across many pages. |
||
| type: "FeatureCollection", | ||
| features: [feature as Feature<Polygon | MultiPolygon>], | ||
| }; | ||
| const mask = buildInvertedMask(collection); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor performance nit (low confidence). |
||
| if (!mask) { | ||
| clearAtlasFeatureMask(map); | ||
| return false; | ||
| } | ||
| const source = map.getSource(SOURCE_ID) as GeoJSONSource | undefined; | ||
| if (source) source.setData(mask); | ||
| else map.addSource(SOURCE_ID, { type: "geojson", data: mask }); | ||
| if (!map.getLayer(FILL_LAYER_ID)) { | ||
| map.addLayer({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Confidence: medium — this is a plausible visual regression for the graticule + polygon-mask combination, but I haven't run the app to confirm the labels actually fall in the masked region. |
||
| id: FILL_LAYER_ID, | ||
| type: "fill", | ||
| source: SOURCE_ID, | ||
| metadata: { "geolibre:internal": true }, | ||
| paint: { | ||
| "fill-color": "#ffffff", | ||
|
Comment on lines
+54
to
+55
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mask fill is a hardcoded Confidence: low — this may well be an intentional, theme-independent print-composition choice (print output is typically a light background regardless of app theme). |
||
| "fill-opacity": 0.7, | ||
| "fill-outline-color": "rgba(0, 0, 0, 0)", | ||
| }, | ||
| }); | ||
| } | ||
| return true; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage gap (low-medium confidence). This new module has no accompanying test file. The codebase already has a precedent for testing this exact kind of MapLibre-mutating code with a fake |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style nit: these two new strings use a curly apostrophe (
’) in "l’entité"/"l’atlas", while the rest of this file consistently uses a straight apostrophe ('), e.g.filterToPageHintright above uses "S'applique", "dataChart.noNumericFields" uses "n'a aucun", etc. Same foratlas.maskOutside/maskOutsideHintbelow. Worth normalizing for consistency with the rest of the catalog.Confidence: low — purely cosmetic, doesn't affect functionality.