@Copilot commented on this pull request.
## Pull request overview
This PR migrates the minimap functionality from Leaflet to MapLibre GL,
introducing a new modular architecture for map styles while maintaining
backward compatibility with the existing layer system. The changes extract
inline style definitions into reusable functions and adapt the minimap
rendering to use MapLibre instead of Leaflet.
- Extracts map style definitions into a centralized `maplibre.styles.js` file
with factory functions
- Refactors language setting logic into a shared `OSM.MapLibre.setMapLanguage`
utility
- Replaces Leaflet-based minimaps with MapLibre GL minimaps in the layer
switcher
### Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7
comments.
<details>
<summary>Show a summary per file</summary>
| File | Description |
| ---- | ----------- |
| app/assets/javascripts/maplibre.styles.js | Introduces centralized style
definitions for all map layers (raster and vector), with factory functions that
generate MapLibre style objects |
| app/assets/javascripts/maplibre.map.js | Updates require statements to
include the new styles and i18n modules (previously had inline Mapnik style
definition) |
| app/assets/javascripts/maplibre.i18n.js | Adds shared `setMapLanguage`
function for setting map language based on user preferences, with support for
asynchronous map loading |
| app/assets/javascripts/leaflet.maptiler.js | Refactors to use the new shared
`setMapLanguage` utility instead of duplicated language-setting code |
| app/assets/javascripts/leaflet.map.js | Stores the layer type identifier in
`layerOptions.leafletOsmId` for use in minimap style selection |
| app/assets/javascripts/leaflet.layers.js | Migrates minimap implementation
from Leaflet to MapLibre GL, using the new style factory functions and
MapLibre's map initialization API |
</details>
---
💡 <a
href="/openstreetmap/openstreetmap-website/new/master/.github/instructions?filename=*.instructions.md"
class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add
Copilot custom instructions</a> for smarter, more guided reviews. <a
href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot";
class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how
to get started</a>.
> OSM.MapLibre.Locale = {
"GeolocateControl.FindMyLocation":
OSM.i18n.t("javascripts.map.locate.title"),
"GeolocateControl.LocationNotAvailable":
OSM.i18n.t("javascripts.map.locate.not_available"),
"NavigationControl.ZoomIn": OSM.i18n.t("javascripts.map.zoom.in"),
"NavigationControl.ZoomOut": OSM.i18n.t("javascripts.map.zoom.out"),
"Popup.Close": OSM.i18n.t("javascripts.map.popup.close")
};
+
The `OSM.MapLibre.setMapLanguage` function lacks documentation. Consider adding
a JSDoc comment to explain its purpose, parameters, and the logic for matching
preferred languages with supported languages. This is especially important
since the function has recursive behavior (calling itself after the map loads)
and complex language matching logic.
```suggestion
/**
* Sets the map's label language based on the user's preferred languages and
* the languages supported by the current map style.
*
* If the map style has not finished loading, this function waits for the
* `"load"` event and then calls itself again once the style is available.
*
* The function retrieves the list of supported languages from the style
* (`map.style.getLanguage()`), then iterates over `OSM.preferred_languages`
* in order. Each preferred language is normalized by lowercasing and
* replacing `-` with `_` before being compared to the supported languages.
* The first exact match found is applied via `map.setLanguage`.
*
* @param {Object} map - The MapLibre map instance whose style and language
* will be updated. It is expected to expose `style.loaded()`,
* `style.getLanguage()`, `once(event, handler)` and `setLanguage(language)`.
*/
```
> @@ -0,0 +1,36 @@
+OSM.MapLibre.Styles.Mapnik = () => createRasterStyle(19,
"https://tile.openstreetmap.org/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.CyclOSM = () => createRasterStyle(20,
"https://{abc}.tile-cyclosm.openstreetmap.fr/cyclosm/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.CycleMap = (options) => createRasterStyle(21,
`https://{abc}.tile.thunderforest.com/cycle/{z}/{x}/{y}.png?apikey=${options.apikey}`);
+OSM.MapLibre.Styles.TransportMap = (options) => createRasterStyle(21,
`https://{abc}.tile.thunderforest.com/transport/{z}/{x}/{y}.png?apikey=${options.apikey}`);
+OSM.MapLibre.Styles.TransportDarkMap = (options) => createRasterStyle(21,
`https://{abc}.tile.thunderforest.com/transport-dark/{z}/{x}/{y}.png?apikey=${options.apikey}`);
+OSM.MapLibre.Styles.OPNVKarte = () => createRasterStyle(18,
"https://tileserver.memomaps.de/tilegen/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.HOT = () => createRasterStyle(20,
"https://tile-{abc}.openstreetmap.fr/hot/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.TracestrackTopo = (options) => createRasterStyle(19,
`https://tile.tracestrack.com/topo__/{z}/{x}/{y}.webp?key=${options.apikey}`);
+OSM.MapLibre.Styles.GPS = (options) => createRasterStyle(21,
"https://gps.tile.openstreetmap.org/lines/{z}/{x}/{y}.png";);
The `GPS` style function is defined but doesn't use the `options` parameter.
Since this function accepts `options` as a parameter (unlike `Mapnik` which
doesn't), either the parameter should be removed if not needed, or if it's
intended for future use with API keys, this should be documented.
```suggestion
OSM.MapLibre.Styles.GPS = () => createRasterStyle(21,
"https://gps.tile.openstreetmap.org/lines/{z}/{x}/{y}.png";);
```
> @@ -42,8 +52,14 @@ L.OSM.layers = function (options) {
setView();
}
- function setView(options) {
- miniMap.setView(map.getCenter(), Math.max(map.getZoom() - 2, 0),
options);
+ function setView(animate = true) {
+ const center = map.getCenter();
+ const zoom = Math.max(Math.floor(map.getZoom() - 3), -1);
The zoom level is calculated as `Math.floor(map.getZoom() - 3)`, which can
result in negative values (minimum -1). While MapLibre might handle negative
zoom gracefully, the previous Leaflet implementation used
`Math.max(map.getZoom() - 2, 0)` to ensure a minimum zoom of 0. The change from
`-2` to `-3` and allowing negative zoom should be intentional, but the
inconsistency in allowing zoom -1 might cause unexpected behavior.
```suggestion
const zoom = Math.max(Math.floor(map.getZoom() - 3), 0);
```
> + const style = OSM.MapLibre.Styles[styleId](layer.options);
+ const miniMap = new maplibregl.Map({
+ container,
+ style,
+ interactive: false,
+ attributionControl: false,
+ fadeDuration: 0
+ });
The minimap should handle errors when the style fails to load (e.g., missing
API keys for layers that require them). Since the style is retrieved
dynamically via `OSM.MapLibre.Styles[styleId](layer.options)`, if the styleId
doesn't exist or the function throws an error, the minimap creation will fail
without proper error handling. Consider adding a try-catch block or error event
listener.
```suggestion
// Ensure the style function exists before calling it
const styleFn = OSM.MapLibre && OSM.MapLibre.Styles &&
OSM.MapLibre.Styles[styleId];
if (typeof styleFn !== "function") {
console.error("Failed to initialize minimap: unknown or invalid
styleId", {
styleId: styleId,
layerOptions: layer.options
});
return;
}
let miniMap;
try {
const style = styleFn(layer.options);
miniMap = new maplibregl.Map({
container,
style,
interactive: false,
attributionControl: false,
fadeDuration: 0
});
} catch (e) {
console.error("Failed to create minimap instance", e);
return;
}
// Log runtime style/map errors instead of failing silently
if (miniMap && typeof miniMap.on === "function") {
miniMap.on("error", function (event) {
console.error("Minimap encountered an error", event);
});
}
```
> + if (OSM.isDarkMap() && L.OSM[leafletOsmDarkId]) {
+ layerOptions.leafletOsmId = leafletOsmDarkId;
+ } else if (L.OSM[leafletOsmId]) {
+ layerOptions.leafletOsmId = leafletOsmId;
+ } else {
+ layerOptions.leafletOsmId = "TileLayer";
+ }
+
+ const layerConstructor = L.OSM[layerOptions.leafletOsmId];
The variable `leafletOsmId` is stored in `layerOptions` but this creates
confusion since the value is now used for MapLibre styles, not Leaflet layers.
Consider renaming this to `maplibreStyleId` or `styleId` to better reflect its
new purpose in the MapLibre migration.
```suggestion
let leafletLayerId;
if (OSM.isDarkMap() && L.OSM[leafletOsmDarkId]) {
leafletLayerId = leafletOsmDarkId;
} else if (L.OSM[leafletOsmId]) {
leafletLayerId = leafletOsmId;
} else {
leafletLayerId = "TileLayer";
}
const layerConstructor = L.OSM[leafletLayerId];
```
> @@ -0,0 +1,36 @@
+OSM.MapLibre.Styles.Mapnik = () => createRasterStyle(19,
"https://tile.openstreetmap.org/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.CyclOSM = () => createRasterStyle(20,
"https://{abc}.tile-cyclosm.openstreetmap.fr/cyclosm/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.CycleMap = (options) => createRasterStyle(21,
`https://{abc}.tile.thunderforest.com/cycle/{z}/{x}/{y}.png?apikey=${options.apikey}`);
+OSM.MapLibre.Styles.TransportMap = (options) => createRasterStyle(21,
`https://{abc}.tile.thunderforest.com/transport/{z}/{x}/{y}.png?apikey=${options.apikey}`);
+OSM.MapLibre.Styles.TransportDarkMap = (options) => createRasterStyle(21,
`https://{abc}.tile.thunderforest.com/transport-dark/{z}/{x}/{y}.png?apikey=${options.apikey}`);
+OSM.MapLibre.Styles.OPNVKarte = () => createRasterStyle(18,
"https://tileserver.memomaps.de/tilegen/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.HOT = () => createRasterStyle(20,
"https://tile-{abc}.openstreetmap.fr/hot/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.TracestrackTopo = (options) => createRasterStyle(19,
`https://tile.tracestrack.com/topo__/{z}/{x}/{y}.webp?key=${options.apikey}`);
+OSM.MapLibre.Styles.GPS = (options) => createRasterStyle(21,
"https://gps.tile.openstreetmap.org/lines/{z}/{x}/{y}.png";);
+OSM.MapLibre.Styles.ShortbreadColorful = (options) =>
`https://vector.openstreetmap.org/styles/shortbread/${options.styleName}`;
+OSM.MapLibre.Styles.ShortbreadEclipse = (options) =>
`https://vector.openstreetmap.org/styles/shortbread/${options.styleName}`;
+OSM.MapLibre.Styles.OpenMapTiles = (options) =>
`https://api.maptiler.com/maps/openstreetmap/style.json?key=${options.apikey}`;
+
The `createRasterStyle` function lacks documentation. Consider adding a JSDoc
comment explaining the purpose of the function, the parameters (maxzoom,
tileUrl), and the return value (MapLibre style object). This would improve code
maintainability, especially since this function is central to the raster style
creation.
```suggestion
/**
* Creates a MapLibre raster style configuration for a given raster tile source.
*
* If the tile URL template contains the `{abc}` placeholder, it will be
expanded
* into separate URLs for the `a`, `b`, and `c` subdomains.
*
* @param {number} maxzoom - Maximum zoom level supported by the raster tile
source.
* @param {string} tileUrl - URL template for raster tiles, optionally
containing `{abc}`.
* @returns {object} A MapLibre Style Specification object configured for a
single raster source.
*/
```
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6683#pullrequestreview-3633170084
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/6683/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
![https://tile.openstreetmap.org/{z}/{x}/{y}.png"](https://tile.openstreetmap.org/{z}/{x}/{y}.png"){kind=link}
![https://{abc}.tile-cyclosm.openstreetmap.fr/cyclosm/{z}/{x}/{y}.png"](https://{abc}.tile-cyclosm.openstreetmap.fr/cyclosm/{z}/{x}/{y}.png"){kind=link}
![https://tileserver.memomaps.de/tilegen/{z}/{x}/{y}.png"](https://tileserver.memomaps.de/tilegen/{z}/{x}/{y}.png"){kind=link}
![https://tile-{abc}.openstreetmap.fr/hot/{z}/{x}/{y}.png"](https://tile-{abc}.openstreetmap.fr/hot/{z}/{x}/{y}.png"){kind=link}
![https://gps.tile.openstreetmap.org/lines/{z}/{x}/{y}.png"](https://gps.tile.openstreetmap.org/lines/{z}/{x}/{y}.png"){kind=link}