Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 25 additions & 0 deletions component/src/charts/__tests__/graph-chart.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,31 @@ describe("GraphChart", () => {
expect(nvlRels[0].type).toBe("knows");
});

it("gives uncolored edges an explicit mid-grey in dark mode (#1154)", () => {
document.documentElement.classList.add("dark");
try {
render(<GraphChart nodes={sampleNodes} edges={sampleEdges} />);
const nvlRels = capturedProps.rels as NvlRelationship[];
// NVL's default relationship grey nearly vanishes on the dark canvas.
expect(nvlRels[0].color).toBe("#6a7180");
} finally {
document.documentElement.classList.remove("dark");
}
});

it("normalizes explicit edge colors to hex for NVL (#1157)", () => {
render(
<GraphChart
nodes={sampleNodes}
edges={[
{ source: "1", target: "2", label: "x", color: "hsl(0, 100%, 50%)" },
]}
/>,
);
const nvlRels = capturedProps.rels as NvlRelationship[];
expect(nvlRels[0].color).toBe("#ff0000");
});

// --- Layout mapping ---

it("uses forceDirected layout by default", () => {
Expand Down
28 changes: 28 additions & 0 deletions component/src/charts/__tests__/theme-defaults.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,34 @@ describe.each(["neoboard-light", "neoboard-dark"])(
expect(label.textBorderWidth).toBe(0);
});

it("pie slice labels use the theme foreground with no halo (#1154)", () => {
// Same failure mode as bar: ECharts' default slice-label style is a dark
// fill with a light stroke — black-on-dark in dark mode.
const pie = theme.pie as Record<string, Record<string, unknown>>;
const label = pie.label as Record<string, unknown>;
const fg = (theme.textStyle as Record<string, unknown>).color;
expect(label.color).toBe(fg);
expect(label.textBorderWidth).toBe(0);
});

it("radar indicator labels use a readable theme color (#1154)", () => {
// ECharts radar axisName does NOT inherit the global textStyle — its
// dim default gray is hard to read on the dark canvas.
const radar = theme.radar as Record<string, Record<string, unknown>>;
const axisName = radar.axisName as Record<string, unknown>;
expect(axisName.color).toBe(
(theme.legend as { textStyle: { color: string } }).textStyle.color,
);
});
Comment on lines +92 to +95

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Duplicate argument passed to toBe() — TS compile error.

Both new assertions pass the same expression twice into .toBe(...), e.g.:

expect(axisName.color).toBe(
  (theme.legend as { textStyle: { color: string } }).textStyle.color,
  (theme.legend as { textStyle: { color: string } }).textStyle.color,
);

toBe takes a single argument; TypeScript will reject the extra argument, breaking the build/test run. Same duplication in the visualMap test.

🐛 Proposed fix
       expect(axisName.color).toBe(
-        (theme.legend as { textStyle: { color: string } }).textStyle.color,
         (theme.legend as { textStyle: { color: string } }).textStyle.color,
       );
       expect((vm.textStyle as Record<string, unknown>).color).toBe(
-        (theme.legend as { textStyle: { color: string } }).textStyle.color,
         (theme.legend as { textStyle: { color: string } }).textStyle.color,
       );

Also applies to: 100-103

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@component/src/charts/__tests__/theme-defaults.test.ts` around lines 92 - 95,
The assertions in theme-defaults.test.ts are passing a duplicate second argument
to Jest’s toBe matcher, which causes a TypeScript compile error. Update the
affected expectations in the axisName and visualMap tests so each
expect(...).toBe(...) call receives only the single intended value, using the
existing theme.legend textStyle color expression once in each assertion.


it("visualMap legend text uses a readable theme color (#1154)", () => {
// Choropleth's visualMap text doesn't inherit textStyle either.
const vm = theme.visualMap as Record<string, Record<string, unknown>>;
expect((vm.textStyle as Record<string, unknown>).color).toBe(
(theme.legend as { textStyle: { color: string } }).textStyle.color,
);
});

it("lines: 1.5px stroke, round caps, smooth by default", () => {
const line = theme.line as Record<string, Record<string, unknown>>;
expect((line.lineStyle as Record<string, unknown>).width).toBe(1.5);
Expand Down
8 changes: 7 additions & 1 deletion component/src/charts/choropleth-chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { CanvasRenderer } from "echarts/renderers";
import type { EChartsOption } from "echarts";
import { BaseChart } from "./base-chart";
import type { BaseChartProps } from "./types";
import { buildEmptyDataOption, isDark } from "./chart-utils";
import { buildEmptyDataOption, fillLabelStyle, isDark } from "./chart-utils";

echarts.use([
EMapChart,
Expand Down Expand Up @@ -165,12 +165,18 @@ function ChoroplethChart({
label: {
show: showLabels,
fontSize: 9,
// Dark mode: labels sit on near-black no-data regions and dark
// ramp fills, where ECharts' default dark text vanishes — use
// white with a dark shadow (treemap pattern, #1154). Light mode
// keeps the default dark text, which reads on the pale map.
...(isDark() ? fillLabelStyle : {}),
},
emphasis: {
label: {
show: true,
fontSize: 13,
fontWeight: "bold",
...(isDark() ? fillLabelStyle : {}),
},
// Keep the region's data color on hover (don't overwrite it with a
// off-brand gold) — the border + shadow are the hover affordance.
Expand Down
13 changes: 10 additions & 3 deletions component/src/charts/graph-chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -276,14 +276,18 @@
edge: GraphEdge,
index: number,
showRelationshipLabels: boolean,
dark: boolean,
): NvlRelationship {
return {
id: `rel-${edge.source}-${edge.target}-${index}`,
from: edge.source,
to: edge.target,
caption: showRelationshipLabels ? edge.label : undefined,
type: edge.label,
color: edge.color,
// NVL's default relationship grey nearly vanishes on the dark canvas
// (#1154) — give uncolored edges an explicit mid-grey in dark mode.
// Normalize to hex either way; NVL doesn't render hsl() (#1157).
color: edge.color ? toNvlColor(edge.color) : dark ? "#6a7180" : undefined,

Check warning on line 290 in component/src/charts/graph-chart.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Extract this nested ternary operation into an independent statement.

See more on https://sonarcloud.io/project/issues?id=alfredo1996_neoboard&issues=AZ8uM90a8tc1zsIwcC2J&open=AZ8uM90a8tc1zsIwcC2J&pullRequest=1177
};
}

Expand Down Expand Up @@ -422,8 +426,11 @@
);

const nvlRels = useMemo(
() => edges.map((e, i) => toNvlRelationship(e, i, showRelationshipLabels)),
[edges, showRelationshipLabels],
() =>
edges.map((e, i) =>
toNvlRelationship(e, i, showRelationshipLabels, dark),
),
[edges, showRelationshipLabels, dark],
);

const fitGraph = useCallback(() => {
Expand Down
10 changes: 10 additions & 0 deletions component/src/charts/theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ function seriesDefaults(popoverBg: string, border: string, fg: string) {
},
pie: {
itemStyle: { borderWidth: 2, borderColor: "transparent" },
// Slice labels: same dark-fill + light-halo ECharts default as bar —
// unreadable on the dark canvas (#1154). Theme foreground, no halo.
label: { color: fg, textBorderWidth: 0 },
},
tooltip: {
backgroundColor: popoverBg,
Expand Down Expand Up @@ -150,6 +153,10 @@ export function registerNeoboardThemes(
detail: { color: "#14161a" },
title: { color: "#666d7a" },
},
// Radar axisName and visualMap text don't inherit the global textStyle —
// their dim ECharts defaults are hard to read on the dark canvas (#1154).
radar: { axisName: { color: "#666d7a" } },
visualMap: { textStyle: { color: "#666d7a" } },
...seriesDefaults("#ffffff", "#e5e7eb", "#14161a"),
});

Expand All @@ -170,6 +177,9 @@ export function registerNeoboardThemes(
detail: { color: "#f3f4f6" },
title: { color: "#959ba7" },
},
// See the light theme note — radar/visualMap text needs explicit color.
radar: { axisName: { color: "#959ba7" } },
visualMap: { textStyle: { color: "#959ba7" } },
...seriesDefaults("#181b20", "#262931", "#f3f4f6"),
});
}
15 changes: 13 additions & 2 deletions component/src/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,24 @@
.dark .leaflet-control-zoom a:hover {
background-color: hsl(220 13% 17%);
}
.dark .leaflet-control-attribution {
/* Needs .leaflet-container in the selector: Leaflet's own
`.leaflet-container .leaflet-control-attribution { background: … }` ties a
bare `.dark .leaflet-control-attribution` on specificity and loads later
(map-chart imports leaflet.css lazily), so Leaflet's white strip won —
leaving amber links on a white bar in dark mode (#1154). */
.dark .leaflet-container .leaflet-control-attribution {
background-color: hsl(220 13% 8% / 0.8);
color: hsl(220 9% 62%);
}
.dark .leaflet-control-attribution a {
.dark .leaflet-container .leaflet-control-attribution a {
color: hsl(38 95% 60%);
}
/* Disabled zoom button (e.g. − at min zoom) — Leaflet's light-grey disabled
style glares against the dark control stack (#1154). */
.dark .leaflet-bar a.leaflet-disabled {
background-color: hsl(220 13% 8%);
color: hsl(220 9% 40%);

Check warning on line 41 in component/src/index.css

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Text does not meet the minimal contrast requirement with its background.

See more on https://sonarcloud.io/project/issues?id=alfredo1996_neoboard&issues=AZ8uM95N8tc1zsIwcC2K&open=AZ8uM95N8tc1zsIwcC2K&pullRequest=1177
}
.dark .leaflet-popup-content-wrapper,
.dark .leaflet-popup-tip {
background-color: hsl(220 13% 11%);
Expand Down
Loading