@hlfan commented on this pull request.
Apart from what I've noted elsewhere, I think the rest of the comfort features
can wait until another PR when this is deployed.
> + &:first-child {
+ border-start-start-radius: 4px;
+ }
+
+ &:last-child {
+ border-end-start-radius: 4px;
+ }
This needs to override the `button:focus` state as well:
<img width="10%"
src="https://github.com/user-attachments/assets/d6bd2492-eccf-4720-9e7d-49c8e691d758";
/>
> + const use1 = document.createElementNS("http://www.w3.org/2000/svg";, "use");
+ use1.setAttributeNS("http://www.w3.org/1999/xlink";, "xlink:href",
"#pin-shadow");
+
+ const use2 = document.createElementNS("http://www.w3.org/2000/svg";, "use");
+ use2.setAttributeNS("http://www.w3.org/1999/xlink";, "xlink:href",
`#pin-${icon}`);
```suggestion
const use1 = document.createElementNS("http://www.w3.org/2000/svg";, "use");
use1.setAttribute("href", "#pin-shadow");
const use2 = document.createElementNS("http://www.w3.org/2000/svg";, "use");
use2.setAttribute("href", `#pin-${icon}`);
```
Others might further suggest using more jQuery here, but since there won't be a
shorthand for `document.createElementNS` without Leaflet, I'd leave it as is.
> + const popup = new maplibregl.Popup()
+ .setHTML(user.description);
Right now, this makes the pop-up tip cover the pin. In the Leaflet map, the
pop-up is positioned higher, 10px above the center of the white pin icon.
> + for (const ctrl of this.controls) {
+ if (typeof ctrl.onRemove === "function") {
+ ctrl.onRemove();
+ }
+ }
```suggestion
for (const ctrl of this.controls) ctrl.onRemove?.();
```
On app/assets/javascripts/maplibre.i18n.js:
Eventually, we'd probably wanna overwrite `map._getUIString`, but I guess this
is fine for now.
> +.maplibregl-popup-tip {
+ border-top-color: rgb(var(--bs-body-bg-rgb)) !important;
+}
Since maplibre tries to keep the pop-up in the map window, all directions are
necessary:
```suggestion
@each $anchor, $border in (
left: right,
right: left,
top: bottom,
bottom: top
) {
.maplibregl-popup#{'[class*="anchor-#{$anchor}"]'} .maplibregl-popup-tip {
border-#{$border}-color: var(--bs-body-bg);
}
}
```
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6504#pullrequestreview-3532811749
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/6504/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
