fix(swipe): correct layer order, basemap label, and pause behavior (#842)#859
Conversation
) The Layer Swipe panel disagreed with the main interface in three ways: - Its LEFT/RIGHT lists were in reverse stack order, and toggling Swipe Enabled off tore down the whole split view instead of just locking the slider. Both are fixed upstream in maplibre-gl-swipe 0.10.1, which this bumps to: the lists now mirror the layer manager (top layer first) and a paused swipe freezes the comparison in place with a locked slider. - The grouped base layer was labeled "Basemap" while the main layer manager calls it "Background". The panel relabeling helper now uses "Background" to match. Fixes #842
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
More reviews will be available in 26 minutes and 3 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Mirror the main layer manager, which labels the base layer "Background" | ||
| // rather than "Basemap" (see issue #842). |
There was a problem hiding this comment.
CLAUDE.md says comments must be at most one short line, and must not reference issue numbers (those belong in the PR description and rot over time). The two-line block and the (see issue #842) citation both violate those rules.
| // Mirror the main layer manager, which labels the base layer "Background" | |
| // rather than "Basemap" (see issue #842). | |
| // Matches the label used by the main layer manager for the grouped base layer. |
There was a problem hiding this comment.
Fixed the comment: it is now a single short line with no issue reference (899b006). On the i18n note, agreed it is the same hardcoded-English pattern the file already uses for its other labels; routing the translated base-layer label through the __GEOLIBRE_LAYER_LABELS__ bridge is a good follow-up and I will open a separate issue for it rather than expand this fix.
There was a problem hiding this comment.
Opened #860 to track the localization follow-up.
|
|
||
| // Mirror the main layer manager, which labels the base layer "Background" | ||
| // rather than "Basemap" (see issue #842). | ||
| const SWIPE_BASEMAP_LABEL = "Background"; |
There was a problem hiding this comment.
i18n gap (medium confidence). The main layer manager renders this label through t("layers.background") (LayerPanel.tsx:2075). Because swipe-style.ts runs outside the React tree it can't call t(), so the string is hardcoded in English. In a non-English locale the swipe panel will always show "Background" while the sidebar shows the translated label.
The existing __GEOLIBRE_LAYER_LABELS__ / geolibre-layer-labels-change bridge (populated by map-controller.ts) is the natural extension point: the publisher could include a __basemap__ entry keyed to the already-translated string, letting this file stay DOM-only while picking up the correct locale. That is out of scope here, but a follow-up issue would keep the two surfaces in sync across locales.
No code change required for this PR; leaving as a note.
Code reviewBugs / Logic ErrorsNone found. The three behavioural fixes (inverted layer order, pause behavior) are correctly delegated to the upstream SecurityNothing to flag. All DOM writes in PerformanceNothing to flag. Quality
CLAUDE.md
Verified clean
|
| layerId === "__basemap__" ? "Basemap" : `${displayName} (${layerId})`; | ||
| layerId === "__basemap__" | ||
| ? SWIPE_BASEMAP_LABEL | ||
| : `${displayName} (${layerId})`; |
There was a problem hiding this comment.
Nit – tooltip is now identical to the label for the basemap
For every other layer the tooltip is "displayName (layerId)", giving a hover hint with the raw layer ID. For __basemap__ the tooltip becomes "Background" – the same text as the visible label – so hovering adds no information.
Exposing the internal sentinel __basemap__ directly would be confusing, but a short disambiguating phrase would make the tooltip meaningful without leaking internals:
| : `${displayName} (${layerId})`; | |
| ? `${SWIPE_BASEMAP_LABEL} (base layer)` |
Low urgency – the pre-existing code had the same pattern (just with "Basemap" for both label and tooltip) – but worth fixing while this line is in motion.
Code reviewScope: 14 additions / 9 deletions across BugsNone found. The constant extraction in SecurityNone found. No user input is evaluated; the DOM writes are plain text assignments ( PerformanceNone found. The MutationObserver + rAF debounce pattern is unchanged; the label fix is inside the existing Quality
CLAUDE.md
Package bump (
|
1 participant
Summary
Fixes the three Layer Swipe panel inconsistencies reported in #842:
LEFT LAYERS/RIGHT LAYERSlists were in reverse stack order (bottom layer at the top). They now mirror the main layer manager, with the top-most layer first and the base layer at the bottom.Basemapwhile the main interface calls itBackground. The swipe panel relabeling helper now usesBackgroundto match.Swipe Enabledoff tore down the entire split-screen comparison. It now freezes the comparison in place and locks the slider (dragging disabled, handle dimmed), keeping the side-by-side view and its layer/orientation controls meaningful.Changes
maplibre-gl-swipeto^0.10.1, which carries the order and pause fixes upstream (fix: mirror host layer order and keep the comparison view when paused maplibre-gl-swipe#18, released as v0.10.1).swipe-style.ts: the panel label helper returnsBackgroundfor the grouped base layer (label and tooltip), matching the layer manager.Testing
Beta, Alpha, Backgroundtop-to-bottom, matching the sidebar.Background.Swipe Enabledoff keeps the split view on screen with a locked slider; toggling on restores dragging.npm run buildandpre-commit run --files(eslint + npm build) pass.Fixes #842