Skip to content

feat(colorbar): stack-order fix, stacking direction, and resizable panel (#884) #901

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.

Sign up for GitHub

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

Merged
giswqs merged 5 commits into from
Jun 26, 2026
Merged

feat(colorbar): stack-order fix, stacking direction, and resizable panel (#884) #901

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e04dbf7
fix(colorbar): keep stack order on update and add stacking direction …
giswqs Jun 25, 2026
f717d5e
test(colorbar): guard stackOrientation round-trip in project state
giswqs Jun 25, 2026
2e569e9
Address Claude review feedback
giswqs Jun 26, 2026
5e2205e
Address Claude review feedback
giswqs Jun 26, 2026
4ada432
feat(colorbar): make the colorbar panel resizable with corner grips
giswqs Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/geolibre-desktop/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"maplibre-gl": "^5.24.0",
"maplibre-gl-3d-tiles": "^0.5.3",
"maplibre-gl-basemap-control": "^0.9.0",
"maplibre-gl-components": "^0.25.1",
"maplibre-gl-components": "^0.25.2",
"maplibre-gl-duckdb": "^0.2.3",
"maplibre-gl-earth-engine": "^0.4.2",
"maplibre-gl-enviroatlas": "^0.1.1",
Expand Down
16 changes: 8 additions & 8 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/plugins/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"maplibre-gl": "^5.24.0",
"maplibre-gl-3d-tiles": "^0.5.3",
"maplibre-gl-basemap-control": "^0.9.0",
"maplibre-gl-components": "^0.25.1",
"maplibre-gl-components": "^0.25.2",
"maplibre-gl-duckdb": "^0.2.3",
"maplibre-gl-earth-engine": "^0.4.2",
"maplibre-gl-enviroatlas": "^0.1.1",
Expand Down
6 changes: 5 additions & 1 deletion packages/plugins/src/plugins/maplibre-components.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,7 @@ interface ComponentColorbarGuiState extends ComponentColorbarGuiEntryState {
hasColorbar: boolean;
selectedColorbarIndex: number;
colorbars: ComponentColorbarGuiEntryState[];
stackOrientation: "horizontal" | "vertical";
}

interface ComponentLegendItem {
Expand Down Expand Up @@ -1431,7 +1432,8 @@ function normalizeComponentsProjectState(
};
}

function normalizeColorbarState(
/** @internal Exported only so the project-state normalizer can be unit-tested. */

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor wording nit: normalizeColorbarState is one of the project-state normalizers, so the phrase "so the project-state normalizer can be unit-tested" is slightly circular. Consider:

Suggested change
/** @internal Exported only so the project-state normalizer can be unit-tested. */
/** @internal Exported only for unit testing. */

@giswqs giswqs Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this open for a maintainer call. These three round-3 comments are all subjective style nits (wording of an @internal note, splitting one assertion group into separate it() blocks, and an informational observation about idempotent deepEqual) with no functional or correctness impact. The prior round's comment-block guidance also reversed itself between rounds, so I'm stopping here rather than churning the branch on marginal stylistic preferences. Happy to apply any of these if you'd like.

export function normalizeColorbarState(
Comment thread
giswqs marked this conversation as resolved.
state: unknown
): ComponentColorbarGuiState | undefined {
if (!state || typeof state !== "object") return undefined;
Expand All @@ -1452,6 +1454,8 @@ function normalizeColorbarState(
hasColorbar: colorbars.length > 0,
selectedColorbarIndex,
colorbars,
stackOrientation:
candidate.stackOrientation === "horizontal" ? "horizontal" : "vertical",
Comment thread
giswqs marked this conversation as resolved.
Comment thread
giswqs marked this conversation as resolved.
};
}

Expand Down
66 changes: 66 additions & 0 deletions tests/colorbar-state-normalization.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import assert from "node:assert/strict";
import { describe, it } from "node:test";
import { normalizeColorbarState } from "../packages/plugins/src/plugins/maplibre-components.ts";

describe("normalizeColorbarState stackOrientation", () => {
it("keeps a horizontal stack orientation", () => {
const normalized = normalizeColorbarState({
visible: true,
colorbars: [],
stackOrientation: "horizontal",
});
assert.equal(normalized?.stackOrientation, "horizontal");
});

it("returns undefined for null/undefined/non-object input", () => {
assert.equal(normalizeColorbarState(null), undefined);
assert.equal(normalizeColorbarState(undefined), undefined);
assert.equal(normalizeColorbarState("nope"), undefined);
});
Comment on lines +15 to +19

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (test quality): three independent assertions in one it() means a failure message identifies the test but not the failing case. Splitting each into its own it() gives a more precise failure signal:

Suggested change
it("returns undefined for null/undefined/non-object input", () => {
assert.equal(normalizeColorbarState(null), undefined);
assert.equal(normalizeColorbarState(undefined), undefined);
assert.equal(normalizeColorbarState("nope"), undefined);
});
it("returns undefined for null input", () => {
assert.equal(normalizeColorbarState(null), undefined);
});
it("returns undefined for undefined input", () => {
assert.equal(normalizeColorbarState(undefined), undefined);
});
it("returns undefined for non-object input", () => {
assert.equal(normalizeColorbarState("nope"), undefined);
});

@giswqs giswqs Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this open for a maintainer call. These three round-3 comments are all subjective style nits (wording of an @internal note, splitting one assertion group into separate it() blocks, and an informational observation about idempotent deepEqual) with no functional or correctness impact. The prior round's comment-block guidance also reversed itself between rounds, so I'm stopping here rather than churning the branch on marginal stylistic preferences. Happy to apply any of these if you'd like.


it("defaults missing stack orientation to vertical (backward compat)", () => {
const normalized = normalizeColorbarState({ visible: true, colorbars: [] });
Comment thread
giswqs marked this conversation as resolved.
assert.equal(normalized?.stackOrientation, "vertical");
});

it("coerces an unknown stack orientation to vertical", () => {
const normalized = normalizeColorbarState({
visible: true,
colorbars: [],
stackOrientation: "diagonal",
});
assert.equal(normalized?.stackOrientation, "vertical");
});
Comment thread
giswqs marked this conversation as resolved.
Comment on lines +26 to +33

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "coerces unknown" test only covers a string value ("diagonal"). Project files are arbitrary JSON, so stackOrientation could also arrive as a number, boolean, or array. Since the normalization uses strict equality (=== "horizontal"), these all correctly fall through to "vertical" — but a quick additional case would make that intent explicit and guard against a future refactor that adds || typeof candidate.stackOrientation === "number" etc.

Suggested change
it("coerces an unknown stack orientation to vertical", () => {
const normalized = normalizeColorbarState({
visible: true,
colorbars: [],
stackOrientation: "diagonal",
});
assert.equal(normalized?.stackOrientation, "vertical");
});
it("coerces an unknown stack orientation to vertical", () => {
for (const bad of ["diagonal", 42, true, null, [], {}]) {
const normalized = normalizeColorbarState({
visible: true,
colorbars: [],
stackOrientation: bad,
});
assert.equal(normalized?.stackOrientation, "vertical", `expected vertical for ${JSON.stringify(bad)}`);
}
});

@giswqs giswqs Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving open for a maintainer call. The code already handles non-string values correctly via strict === "horizontal" (number/boolean/array all fall through to vertical), so this is a marginal coverage nit rather than a defect. I'm holding off on more test-only churn here since the review has converged to stylistic nits on this file across several rounds.


it("preserves an explicit vertical stack orientation", () => {
const normalized = normalizeColorbarState({
visible: true,
colorbars: [],
stackOrientation: "vertical",
});
assert.equal(normalized?.stackOrientation, "vertical");
});

it("round-trips a horizontal choice through a second normalization", () => {
const once = normalizeColorbarState({
visible: true,
colorbars: [
{
mode: "named",
colormap: "viridis",
customColors: "#440154, #31688e, #21918c, #90d743, #fde725",
vmin: 0,
vmax: 100,
label: "Depth",
units: "",
orientation: "vertical",
colorbarPosition: "bottom-right",
},
],
stackOrientation: "horizontal",
});
Comment thread
giswqs marked this conversation as resolved.
const twice = normalizeColorbarState(once);
assert.equal(twice?.stackOrientation, "horizontal");
assert.deepEqual(twice, once);
});
Comment on lines +44 to +65

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.deepEqual(twice, once) passes correctly (idempotent), but is slightly surprising on first read: the top-level form fields (mode, colormap, label, vmin, vmax, etc.) in once are set to their defaults — not derived from colorbars[0] — because the original input has no top-level form fields. So once.label === "" even though colorbars[0].label === "Depth".

The test is correct, but a short inline comment would help future readers understand why deepEqual holds without surprise:

Suggested change
it("round-trips a horizontal choice through a second normalization", () => {
const once = normalizeColorbarState({
visible: true,
colorbars: [
{
mode: "named",
colormap: "viridis",
customColors: "#440154, #31688e, #21918c, #90d743, #fde725",
vmin: 0,
vmax: 100,
label: "Depth",
units: "",
orientation: "vertical",
colorbarPosition: "bottom-right",
},
],
stackOrientation: "horizontal",
});
const twice = normalizeColorbarState(once);
assert.equal(twice?.stackOrientation, "horizontal");
assert.deepEqual(twice, once);
});
it("round-trips a horizontal choice through a second normalization", () => {
// Top-level form fields in `once` come from defaults (the input has no top-level
// mode/label/etc.), so `once.label` will be "" regardless of colorbars[0].label.
// The deepEqual check verifies that normalizeColorbarState is idempotent once the
// state is already normalized.
const once = normalizeColorbarState({
visible: true,
colorbars: [
{
mode: "named",
colormap: "viridis",
customColors: "#440154, #31688e, #21918c, #90d743, #fde725",
vmin: 0,
vmax: 100,
label: "Depth",
units: "",
orientation: "vertical",
colorbarPosition: "bottom-right",
},
],
stackOrientation: "horizontal",
});
const twice = normalizeColorbarState(once);
assert.equal(twice?.stackOrientation, "horizontal");
assert.deepEqual(twice, once);
});

@giswqs giswqs Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this open for a maintainer call. These three round-3 comments are all subjective style nits (wording of an @internal note, splitting one assertion group into separate it() blocks, and an informational observation about idempotent deepEqual) with no functional or correctness impact. The prior round's comment-block guidance also reversed itself between rounds, so I'm stopping here rather than churning the branch on marginal stylistic preferences. Happy to apply any of these if you'd like.

Comment on lines +44 to +65

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert.deepEqual(twice, once) at the end is a useful idempotency check. One thing to be aware of: the input to the first normalization has no top-level entry-level fields (mode, colormap, etc.), so once gets these from defaults via normalizeColorbarEntry. The deep-equal passes because normalizeColorbarEntry(once) reads those same default values back from once and produces an identical object.

If ComponentColorbarGuiState ever gains a computed/ephemeral field (e.g. a _dirty flag set only by setState), this assertion could start failing for unrelated reasons. It's a minor fragility. If idempotency is the goal, targeted field assertions (assert.equal(twice?.colorbars[0]?.colormap, once?.colorbars[0]?.colormap) etc.) are more resilient — though the current broad check is a net positive for catching regressions.

@giswqs giswqs Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, and it's intentional: the test is an idempotency check on already-normalized state. The top-level form fields come from defaults because the input has no top-level fields, and deepEqual(twice, once) verifies normalization is stable. Leaving open for your call rather than reworking the test further.

});
Loading