From ecc0ad6e90cbe438414e35257a2c5e2adce15548 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 14 Sep 2026 14:47:16 -0700 Subject: [PATCH 1/8] CODAP-1530: explain a legend attribute the display cannot honor Dropping an attribute from a collection more childmost than the plotted cases was accepted and then never mentioned again. The legend returned null, which took the attribute's name and the remove action with it, so the choice became invisible and there was no way back from the graph. Meanwhile the Format palette went on offering a color and a shape per category, because GraphDataConfigurationModel's attributeDescriptionForRole override keeps the description the base filters out. The graph disowned the legend in one place and honored it in another. The legend now renders in that state, carrying the attribute label and a sentence where the keys would be. assignedLegendAttributeID reads the assignment off the description rather than through attributeID, which answers differently by display, and legendAttributeIsInoperable pairs it with the existing allowed-check. The palette drops the per-category rows and keeps the display-wide shape control, which unlike color does still reach these points: getLegendShapeForCase falls back to the display's own shape while getLegendColorForCase overrides with the missing-value color. Its glyph is drawn in that color, so the palette shows what is plotted rather than promising a color the points will not take. The gray points are unchanged. They are the signal that the attribute was accepted but cannot be resolved per point, and the message explains them. Co-Authored-By: Claude Opus 5 --- .../legend/inoperable-legend-message.test.ts | 39 +++++++++++ .../legend/inoperable-legend-message.tsx | 68 +++++++++++++++++++ .../legend/legend-attribute-label.tsx | 4 +- .../components/legend/legend.scss | 6 ++ .../data-display/components/legend/legend.tsx | 15 +++- .../inspector/legend-color-controls.test.tsx | 51 ++++++++++++++ .../inspector/legend-color-controls.tsx | 19 +++++- .../models/data-configuration-model.ts | 20 ++++++ .../graph-data-configuration-model.test.ts | 44 ++++++++++++ v3/src/utilities/translation/lang/en-US.json5 | 4 ++ 10 files changed, 265 insertions(+), 5 deletions(-) create mode 100644 v3/src/components/data-display/components/legend/inoperable-legend-message.test.ts create mode 100644 v3/src/components/data-display/components/legend/inoperable-legend-message.tsx diff --git a/v3/src/components/data-display/components/legend/inoperable-legend-message.test.ts b/v3/src/components/data-display/components/legend/inoperable-legend-message.test.ts new file mode 100644 index 0000000000..38c6399643 --- /dev/null +++ b/v3/src/components/data-display/components/legend/inoperable-legend-message.test.ts @@ -0,0 +1,39 @@ +// jsdom does no text layout, so width is stubbed per character the way the sibling legend tests do. +jest.mock("../../../../hooks/use-measure-text", () => ({ + measureText: (text: string) => text.length * 10, + // also stubbed because labelHeight, pulled in transitively, measures at module load + measureTextExtent: (text: string) => ({ width: text.length * 10, height: 12 }) +})) + +import { wrapTextToWidth } from "./inoperable-legend-message" + +describe("wrapTextToWidth", () => { + it("leaves a string that fits on one line", () => { + expect(wrapTextToWidth("one two", 1000)).toEqual(["one two"]) + }) + + it("breaks between words at the available width", () => { + // 10px per character, so 100px holds ten characters + expect(wrapTextToWidth("aaaa bbbb cccc dddd", 100)).toEqual(["aaaa bbbb", "cccc dddd"]) + }) + + it("rewraps as the width changes", () => { + const text = "aaaa bbbb cccc dddd" + expect(wrapTextToWidth(text, 50)).toEqual(["aaaa", "bbbb", "cccc", "dddd"]) + expect(wrapTextToWidth(text, 150)).toEqual(["aaaa bbbb cccc", "dddd"]) + }) + + it("keeps a word too long to fit rather than dropping or splitting it", () => { + // the legend can be narrower than a single word; overflowing one line beats losing the word + expect(wrapTextToWidth("aaaaaaaaaa bb", 30)).toEqual(["aaaaaaaaaa", "bb"]) + }) + + it("collapses runs of whitespace", () => { + expect(wrapTextToWidth(" one two ", 1000)).toEqual(["one two"]) + }) + + it("returns nothing for an empty string", () => { + expect(wrapTextToWidth("", 1000)).toEqual([]) + expect(wrapTextToWidth(" ", 1000)).toEqual([]) + }) +}) diff --git a/v3/src/components/data-display/components/legend/inoperable-legend-message.tsx b/v3/src/components/data-display/components/legend/inoperable-legend-message.tsx new file mode 100644 index 0000000000..0a0a78b4b6 --- /dev/null +++ b/v3/src/components/data-display/components/legend/inoperable-legend-message.tsx @@ -0,0 +1,68 @@ +import { observer } from "mobx-react-lite" +import { useEffect } from "react" +import { measureText } from "../../../../hooks/use-measure-text" +import { t } from "../../../../utilities/translation/translate" +import { axisGap } from "../../../axis/axis-types" +import { kDataDisplayFont } from "../../data-display-types" +import { useDataConfigurationContext } from "../../hooks/use-data-configuration-context" +import { useDataDisplayLayout } from "../../hooks/use-data-display-layout" +import { labelHeight, padding } from "./categorical-legend-model" +import { IBaseLegendProps } from "./legend-common" + +import "./legend.scss" + +const kLineHeight = 16 + +// Greedy word wrap. SVG text does not wrap, and this message is a sentence rather than a label, so +// it is broken into lines here and drawn as one tspan each. +export function wrapTextToWidth(text: string, maxWidth: number, font = kDataDisplayFont): string[] { + const words = text.split(/\s+/).filter(word => !!word) + if (!words.length) return [] + + const lines: string[] = [] + let line = words[0] + for (const word of words.slice(1)) { + const candidate = `${line} ${word}` + if (measureText(candidate, font) <= maxWidth) { + line = candidate + } else { + lines.push(line) + line = word + } + } + lines.push(line) + return lines +} + +/* + * Stands in for the keys when the assigned legend attribute is one this display cannot honor. + * + * The keys are the wrong thing to draw -- there is no per-category encoding to key -- but drawing + * nothing was worse: the legend collapsed entirely, taking the attribute's name and its remove + * action with it, so the assignment the user made became invisible and unreachable. + */ +export const InoperableLegendMessage = observer(function InoperableLegendMessage( + { layerIndex, setDesiredExtent }: IBaseLegendProps +) { + const dataConfiguration = useDataConfigurationContext() + const dataDisplayLayout = useDataDisplayLayout() + const attrID = dataConfiguration?.assignedLegendAttributeID + const attrName = (attrID && dataConfiguration?.dataset?.attrFromID(attrID)?.name) || "" + // Both gaps, so a long word cannot push the text past the edge the keys stop at. + const maxWidth = Math.max(dataDisplayLayout.tileWidth - 2 * axisGap, 1) + const lines = wrapTextToWidth(t("V3.Legend.attributeNotOperable", { vars: [attrName] }), maxWidth) + + useEffect(() => { + setDesiredExtent(layerIndex, labelHeight + lines.length * kLineHeight + padding + axisGap) + return () => setDesiredExtent(layerIndex, 0) + }, [layerIndex, lines.length, setDesiredExtent]) + + return ( + + {lines.map((line, i) => ( + {line} + ))} + + ) +}) diff --git a/v3/src/components/data-display/components/legend/legend-attribute-label.tsx b/v3/src/components/data-display/components/legend/legend-attribute-label.tsx index fd68aa74f8..b039febbee 100644 --- a/v3/src/components/data-display/components/legend/legend-attribute-label.tsx +++ b/v3/src/components/data-display/components/legend/legend-attribute-label.tsx @@ -25,7 +25,9 @@ export const LegendAttributeLabel = const refreshLegendTitle = useCallback(() => { const dataset = dataConfiguration?.dataset, - attributeID = dataConfiguration?.attributeID('legend'), + // The assigned attribute rather than attributeID('legend'), which reports "" for one this + // display cannot honor -- the label has to name it in order to offer removing it. + attributeID = dataConfiguration?.assignedLegendAttributeID, attributeName = (attributeID ? dataset?.attrFromID(attributeID)?.name : '') ?? '', attributeUnits = (attributeID ? dataset?.attrFromID(attributeID)?.units : '') ?? '', labelFont = vars.labelFont, diff --git a/v3/src/components/data-display/components/legend/legend.scss b/v3/src/components/data-display/components/legend/legend.scss index 7cb57da2b0..b3d63b6f55 100644 --- a/v3/src/components/data-display/components/legend/legend.scss +++ b/v3/src/components/data-display/components/legend/legend.scss @@ -28,6 +28,12 @@ font: 12px sans-serif; } + // Hanging, because the message is positioned by its top edge the way the keys are. + .legend-inoperable-message { + fill: #555555; + dominant-baseline: hanging; + } + // The key shape is listed alongside the rects because this is a type selector: a categorical key // is drawn as a path, and would otherwise lose the border and the softening every other legend // swatch has. diff --git a/v3/src/components/data-display/components/legend/legend.tsx b/v3/src/components/data-display/components/legend/legend.tsx index 85abff7c9a..7a183379bd 100644 --- a/v3/src/components/data-display/components/legend/legend.tsx +++ b/v3/src/components/data-display/components/legend/legend.tsx @@ -7,6 +7,7 @@ import { IDataConfigurationModel } from "../../models/data-configuration-model" import { LegendAttributeLabel } from "./legend-attribute-label" import { CategoricalLegend } from "./categorical-legend" import { ColorLegend } from "./color-legend" +import { InoperableLegendMessage } from "./inoperable-legend-message" import { IBaseLegendProps } from "./legend-common" import { NumericLegend } from "./numeric-legend" @@ -38,18 +39,26 @@ export const Legend = observer(function Legend({ const dataConfiguration = useDataConfigurationContext(), legendID = dataConfiguration?.attributeID("legend"), legendRef = useRef() as React.RefObject - if (!dataConfiguration?.isAttributeAllowedForNonAxisRole(legendID)) return null + /* + * An assigned attribute this display cannot honor still gets a legend -- the label and the + * message, in place of the keys. Returning null for it hid the fact that the drop was accepted, + * and the remove action lives on the label, so there was no way back from the graph. + */ + const isInoperable = !!dataConfiguration?.legendAttributeIsInoperable + if (!isInoperable && !dataConfiguration?.isAttributeAllowedForNonAxisRole(legendID)) return null const attrType = dataConfiguration?.attributeType('legend'), LegendComponent = dataConfiguration && legendComponentManager.getLegendComponent(dataConfiguration) // Only show the legend if there is a legend role specified in the dataConfiguration - return attrType ? ( + return attrType || isInoperable ? ( <> - {LegendComponent && } + {isInoperable + ? + : LegendComponent && } ) : null diff --git a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx index d8f0d485c9..cd90bb5aa0 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx @@ -2,6 +2,7 @@ import { act, fireEvent, render, screen, within } from "@testing-library/react" import userEvent from "@testing-library/user-event" import { scaleQuantize } from "d3" import { featureFlagManager } from "../../../models/feature-flags/feature-flag-manager" +import { missingColor } from "../../../utilities/color-utils" import { LegendColorControls, LegendBinsSelect, LegendBinCountInput, LegendRangeInputs } from "./legend-color-controls" @@ -889,4 +890,54 @@ describe("point shape controls", () => { expect(desc.setPointShape).toHaveBeenCalledWith("diamond") }) }) + + describe("a legend the display cannot honor", () => { + const inoperableConfig = (overrides?: Record) => createMockDataConfig({ + // the graph reaches the palette with the type still resolved, which is why the rows appeared + attributeType: jest.fn(() => "categorical"), + categoryArrayForAttrRole: jest.fn(() => ["cat-a", "cat-b"]), + legendAttributeIsInoperable: true, + ...overrides + }) + + it("offers no per-category rows, since nothing they set can reach the points", () => { + featureFlagManager.setServerConfig({ pointShapes: "on" }) + const desc = createMockDescription() + render() + + expect(screen.queryByTestId("color-swatch-cat-a")).not.toBeInTheDocument() + expect(screen.queryByTestId("color-swatch-cat-b")).not.toBeInTheDocument() + }) + + it("keeps the display-wide shape control, which does still reach them", () => { + // getLegendShapeForCase falls back to the display's own shape in this state + featureFlagManager.setServerConfig({ pointShapes: "on" }) + const desc = createMockDescription() + render() + + expect(screen.getAllByTestId("point-shape-select")).toHaveLength(1) + }) + + it("draws the shape glyph in the missing-value color the points are drawn in", () => { + featureFlagManager.setServerConfig({ pointShapes: "on" }) + const desc = createMockDescription() + render() + + const glyph = within(screen.getByTestId("point-shape-select")).getAllByTestId("point-shape-glyph")[0] + expect(glyph).toHaveStyle({ color: missingColor }) + }) + + it("leaves the rows alone when the legend is one the display can honor", () => { + featureFlagManager.setServerConfig({ pointShapes: "on" }) + const desc = createMockDescription() + render() + + expect(screen.getByTestId("color-swatch-cat-a")).toBeInTheDocument() + }) + }) + }) diff --git a/v3/src/components/data-display/inspector/legend-color-controls.tsx b/v3/src/components/data-display/inspector/legend-color-controls.tsx index e00450eec4..14424402dc 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.tsx @@ -11,6 +11,7 @@ import { AttributeBinningTypes, AttributeBinningType } from "../../../models/sha import { kDefaultHighAttributeColor, kDefaultLowAttributeColor } from "../../../models/shared/data-set-metadata-constants" +import { missingColor } from "../../../utilities/color-utils" import { binBoundaryDecimalPlaces } from "../../../utilities/math-utils" import { PointShape } from "../../../utilities/point-shape-utils" import { t } from "../../../utilities/translation/translate" @@ -151,6 +152,14 @@ export const LegendColorControls = observer(function LegendColorControls( * those, or a shape chosen before the legend was added becomes unreachable while it goes on * governing what is drawn. */ + /* + * The glyph shows what the points are actually drawn as. A legend this display cannot honor draws + * them in the missing-value color whatever the point color says, so tinting the glyph with the + * point color would promise something the plot does not do. + */ + const glyphColor = dataConfiguration.legendAttributeIsInoperable + ? missingColor : displayItemDescription.pointColor + const displayShapeRow = showShape ? (
@@ -169,13 +178,21 @@ export const LegendColorControls = observer(function LegendColorControls(
) : null + /* + * A legend this display cannot honor gets no per-category rows. The graph reaches here with + * attrType still "categorical" -- its attributeDescriptionForRole override keeps the description + * the base filters out -- so without this check the palette offers colors and shapes for + * categories that cannot reach the points. The legend itself says why. + */ + if (dataConfiguration.legendAttributeIsInoperable) return displayShapeRow + if (attrType === "categorical") { return ( ({ + /* + * The legend attribute the user assigned, whether or not this display can honor it. + * + * Read off the description rather than through `attributeID`, which answers differently by + * display: the base filters an unusable legend out, while GraphDataConfigurationModel's + * override does not. The assignment is the user's intent either way, and the legend has to be + * able to name it -- and offer to remove it -- rather than drop it silently. + */ + get assignedLegendAttributeID(): string { + return self._attributeDescriptions.get("legend")?.attributeID ?? "" + }, + /* + * Whether that attribute is one this display cannot color or shape points by, because it lives + * in a collection more childmost than the plotted cases -- a point then stands for several of + * its values at once, which is why those points draw in the missing-value color. + */ + get legendAttributeIsInoperable(): boolean { + const attrID = this.assignedLegendAttributeID + return !!attrID && !self.isAttributeAllowedForNonAxisRole(attrID) + }, _caseHasValidValuesForDescriptions(data: IDataSet, caseID: string, descriptions: AttributeDescriptionsMapSnapshot) { return Object.entries(descriptions).every(([role, {attributeID}]) => { diff --git a/v3/src/components/graph/models/graph-data-configuration-model.test.ts b/v3/src/components/graph/models/graph-data-configuration-model.test.ts index 772a771751..ae4a05c3ec 100644 --- a/v3/src/components/graph/models/graph-data-configuration-model.test.ts +++ b/v3/src/components/graph/models/graph-data-configuration-model.test.ts @@ -1175,6 +1175,50 @@ describe("DataConfigurationModel legend point shapes", () => { expect(tree.config.getLegendShapeForCase(parentCaseId, "star")).toBe("star") }) + describe("an assigned legend attribute this display cannot honor", () => { + // The same hierarchy as the fallback test above: x in the parent, legend left childmost. + const makeLegendChildmost = () => { + tree.data.addAttribute({ id: "xId", name: "x" }) + tree.data.setCaseValues([ + { __id__: "c1", xId: "shared" }, + { __id__: "c2", xId: "shared" } + ]) + tree.config.setAttribute("x", { attributeID: "xId" }) + tree.data.moveAttributeToNewCollection("xId") + } + + it("is reported as inoperable rather than dropped", () => { + /* + * The graph's attributeDescriptionForRole override keeps the description the base filters + * out, so neither attributeID nor attributeType can answer this on its own. + */ + expect(tree.config.legendAttributeIsInoperable).toBe(false) + + makeLegendChildmost() + + expect(tree.config.legendAttributeIsInoperable).toBe(true) + // the assignment is still the user's, and the legend has to be able to name it to remove it + expect(tree.config.assignedLegendAttributeID).toBe("legId") + }) + + it("is not reported inoperable once the plotted cases reach it again", () => { + makeLegendChildmost() + expect(tree.config.legendAttributeIsInoperable).toBe(true) + + // plotting a childmost attribute puts the legend back within reach + tree.config.setAttribute("x", { attributeID: "legId" }) + + expect(tree.config.legendAttributeIsInoperable).toBe(false) + }) + + it("reports nothing assigned when there is no legend attribute", () => { + tree.config.setAttribute("legend", { attributeID: "" }) + + expect(tree.config.assignedLegendAttributeID).toBe("") + expect(tree.config.legendAttributeIsInoperable).toBe(false) + }) + }) + it("falls back to the default when there is no legend attribute", () => { tree.config.setAttribute("legend", { attributeID: "" }) // no category set to consult, so reads resolve rather than returning undefined diff --git a/v3/src/utilities/translation/lang/en-US.json5 b/v3/src/utilities/translation/lang/en-US.json5 index f52e44ae20..6c61434c6a 100644 --- a/v3/src/utilities/translation/lang/en-US.json5 +++ b/v3/src/utilities/translation/lang/en-US.json5 @@ -1671,6 +1671,10 @@ "V3.graphLSRL.showR": "Show r", "V3.graphLSRL.showRSquared": "Show r\u00B2", "V3.Inspector.graphResidualPlot": "Residual Plot", + // Shown where the legend keys would be, when the legend attribute belongs to a collection more + // childmost than the plotted cases. The check is structural -- it does not look at whether the + // values actually differ within a case -- so the wording says "can span". %@ is the attribute. + "V3.Legend.attributeNotOperable": "Each point can span multiple %@ values, so it can't be used to distinguish them.", "V3.ResidualPlot.axisLabel": "Residuals", "V3.ResidualPlot.dataTip.residual": "Residual", "V3.Undo.graph.showResidualPlot": "Undo showing residual plot", From 78ff42fa6364d6da469030ae4bbffa0aba732612 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 14 Sep 2026 15:04:37 -0700 Subject: [PATCH 2/8] CODAP-1530: address Copilot review A deleted attribute leaves its ID in the legend description, and no collection holds it, so the allowed-check said no and the state read as inoperable. The legend would then have explained itself over a name that is no longer there -- where before this branch it correctly showed nothing at all. legendAttributeIsInoperable now requires the attribute to still exist, which is the check getLegendColorForCase already makes for the same reason. Key the wrapped lines by position. Two of them can hold the same text once an attribute name repeats a word or the tile narrows, and a line has no identity apart from where it sits. Co-Authored-By: Claude Opus 5 --- .../components/legend/inoperable-legend-message.tsx | 4 +++- .../data-display/models/data-configuration-model.ts | 9 ++++++++- .../models/graph-data-configuration-model.test.ts | 11 +++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/v3/src/components/data-display/components/legend/inoperable-legend-message.tsx b/v3/src/components/data-display/components/legend/inoperable-legend-message.tsx index 0a0a78b4b6..0c0e74a15c 100644 --- a/v3/src/components/data-display/components/legend/inoperable-legend-message.tsx +++ b/v3/src/components/data-display/components/legend/inoperable-legend-message.tsx @@ -61,7 +61,9 @@ export const InoperableLegendMessage = observer(function InoperableLegendMessage {lines.map((line, i) => ( - {line} + // Keyed by position: a wrapped line has no identity apart from where it sits, and two of + // them can hold the same text once an attribute name repeats a word or the tile narrows. + {line} ))} ) diff --git a/v3/src/components/data-display/models/data-configuration-model.ts b/v3/src/components/data-display/models/data-configuration-model.ts index 6c3631da8c..3801c5ceef 100644 --- a/v3/src/components/data-display/models/data-configuration-model.ts +++ b/v3/src/components/data-display/models/data-configuration-model.ts @@ -265,7 +265,14 @@ export const DataConfigurationModel = types */ get legendAttributeIsInoperable(): boolean { const attrID = this.assignedLegendAttributeID - return !!attrID && !self.isAttributeAllowedForNonAxisRole(attrID) + /* + * Deleting an attribute leaves its ID behind in the description -- see the todo in + * getLegendColorForCase -- and no collection holds it, so the allowed-check below says no. + * Without this the legend would explain itself over a name that is no longer there. + */ + if (!attrID || !self.dataset?.getAttribute(attrID)) return false + + return !self.isAttributeAllowedForNonAxisRole(attrID) }, _caseHasValidValuesForDescriptions(data: IDataSet, caseID: string, descriptions: AttributeDescriptionsMapSnapshot) { diff --git a/v3/src/components/graph/models/graph-data-configuration-model.test.ts b/v3/src/components/graph/models/graph-data-configuration-model.test.ts index ae4a05c3ec..ce14ef9304 100644 --- a/v3/src/components/graph/models/graph-data-configuration-model.test.ts +++ b/v3/src/components/graph/models/graph-data-configuration-model.test.ts @@ -1211,6 +1211,17 @@ describe("DataConfigurationModel legend point shapes", () => { expect(tree.config.legendAttributeIsInoperable).toBe(false) }) + it("does not report a deleted attribute as inoperable", () => { + /* + * Deleting an attribute leaves its ID behind in the legend description (see the todo in + * getLegendColorForCase), and no collection holds it, so the allowed-check says no. Without + * a liveness check that reads as inoperable and the legend renders a nameless message. + */ + tree.config.setAttribute("legend", { attributeID: "goneId" }) + + expect(tree.config.legendAttributeIsInoperable).toBe(false) + }) + it("reports nothing assigned when there is no legend attribute", () => { tree.config.setAttribute("legend", { attributeID: "" }) From ec8c137d97cf5e9f5aa1f4665c901abd7f3f498c Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 14 Sep 2026 15:34:34 -0700 Subject: [PATCH 3/8] CODAP-1530: address Copilot review of the numeric branch The palette was fixed for a categorical legend it cannot honor and left as it was for a numeric one, which is reachable by the same route. A numeric legend supplies a band of colors for the shape glyph's fill, and the fill wins over the color prop, so the glyph showed the legend's colors while every point was drawn in the missing-value color. The band is now empty in that state, which also drops the gradient definition it fed. The binning selector and the flagged bin-count and range inputs are siblings of the palette rather than part of it, gated on the legend being numeric alone, so they stayed visible with nothing to act on. They now carry the same condition. Co-Authored-By: Claude Opus 5 --- .../display-item-format-control.test.tsx | 24 ++++++++++++++ .../inspector/display-item-format-control.tsx | 4 ++- .../inspector/legend-color-controls.test.tsx | 32 +++++++++++++++++++ .../inspector/legend-color-controls.tsx | 7 +++- 4 files changed, 65 insertions(+), 2 deletions(-) diff --git a/v3/src/components/data-display/inspector/display-item-format-control.test.tsx b/v3/src/components/data-display/inspector/display-item-format-control.test.tsx index 9d3e1f8e30..1e1c2461a8 100644 --- a/v3/src/components/data-display/inspector/display-item-format-control.test.tsx +++ b/v3/src/components/data-display/inspector/display-item-format-control.test.tsx @@ -120,6 +120,30 @@ describe("DisplayItemFormatControl", () => { expect(screen.getByTestId("legend-range-inputs")).toBeInTheDocument() }) + it("hides the numeric legend controls for a legend the display cannot honor", () => { + /* + * Every point is drawn in the missing-value color in that state, so binning and range have + * nothing to act on. The palette drops its own rows for it; these are its siblings and need + * the same gate. + */ + featureFlagManager.setServerConfig({ legendBinCount: "on", legendRange: "on" }) + const desc = createMockDescription() + const config = createMockDataConfig({ + attributeType: jest.fn(() => "numeric"), + legendAttributeIsInoperable: true + }) + render( + + ) + + expect(screen.queryByTestId("legend-bins-select")).not.toBeInTheDocument() + expect(screen.queryByTestId("legend-bin-count-input")).not.toBeInTheDocument() + expect(screen.queryByTestId("legend-range-inputs")).not.toBeInTheDocument() + }) + it("hides the flagged numeric legend controls when their flags are off", () => { const desc = createMockDescription() const config = createMockDataConfig({ diff --git a/v3/src/components/data-display/inspector/display-item-format-control.tsx b/v3/src/components/data-display/inspector/display-item-format-control.tsx index 96837e5337..e5c71166cb 100644 --- a/v3/src/components/data-display/inspector/display-item-format-control.tsx +++ b/v3/src/components/data-display/inspector/display-item-format-control.tsx @@ -135,7 +135,9 @@ export const DisplayItemFormatControl = observer(function DisplayItemFormatContr displayItemDescription={displayItemDescription} /> - + {/* Not for a legend this display cannot honor: every point is drawn in the missing-value + color, so binning and range have nothing to act on. */} + diff --git a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx index cd90bb5aa0..0f4a39b286 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx @@ -930,6 +930,38 @@ describe("point shape controls", () => { expect(glyph).toHaveStyle({ color: missingColor }) }) + it("draws no gradient on the glyph for a numeric legend it cannot honor", () => { + /* + * A numeric legend supplies a band of colors for the glyph's fill, and that fill wins over + * the color prop -- so without suppressing it the glyph would show the legend's colors while + * every point is drawn in the missing-value color. + */ + featureFlagManager.setServerConfig({ pointShapes: "on" }) + const desc = createMockDescription() + const config = inoperableConfig({ attributeType: jest.fn(() => "numeric") }) + render() + + // the gradient is applied through this custom property, which overrides the solid color + const glyph = within(screen.getByTestId("point-shape-select")).getAllByTestId("point-shape-glyph")[0] + expect(glyph.style.getPropertyValue("--point-shape-fill")).toBe("") + expect(glyph).toHaveStyle({ color: missingColor }) + }) + + it("still draws the gradient for a numeric legend it can honor", () => { + featureFlagManager.setServerConfig({ pointShapes: "on" }) + const desc = createMockDescription() + const config = inoperableConfig({ + attributeType: jest.fn(() => "numeric"), + legendAttributeIsInoperable: false + }) + render() + + const glyph = within(screen.getByTestId("point-shape-select")).getAllByTestId("point-shape-glyph")[0] + expect(glyph.style.getPropertyValue("--point-shape-fill")).toMatch(/^url\(#/) + }) + it("leaves the rows alone when the legend is one the display can honor", () => { featureFlagManager.setServerConfig({ pointShapes: "on" }) const desc = createMockDescription() diff --git a/v3/src/components/data-display/inspector/legend-color-controls.tsx b/v3/src/components/data-display/inspector/legend-color-controls.tsx index 14424402dc..31cebdac4c 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.tsx @@ -133,7 +133,12 @@ export const LegendColorControls = observer(function LegendColorControls( * quantiled, so points only ever take these discrete colors and a smooth ramp would show shades * nothing in the plot has. Few bins therefore read as visible bands, which is honest. */ - const legendBandColors: string[] = attrType === "numeric" + /* + * No gradient for a legend this display cannot honor: the points are all drawn in the + * missing-value color, so a band of the legend's colors would promise exactly what glyphColor + * below exists to stop promising -- and the gradient wins, since it paints the glyph's fill. + */ + const legendBandColors: string[] = attrType === "numeric" && !dataConfiguration.legendAttributeIsInoperable ? (dataConfiguration.legendNumericColorScale?.range() ?? []) : [] const legendBandStops = legendBandColors.flatMap((bandColor, i) => [ From ffc4e54edf8bea6280f0a1d18b84c469ff3d0e57 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 14 Sep 2026 15:55:10 -0700 Subject: [PATCH 4/8] CODAP-1530: scope the palette's gate to displays that gray their points legendAttributeIsInoperable answers "the assignment cannot be honored", which is what the legend area needs: the label and the remove action go missing on a map too, so it renders the message there as well. The palette was asking a narrower question and using the same answer. A map layer uses the base configuration, which filters the unusable assignment out of attributeID, so nothing consults the legend and its points keep the display's own color -- while the palette hid the color control and drew the glyph gray. On a polygon layer there is no shape control to fall back on, so the section lost its only control. allPointsTakeMissingColor adds the missing condition: the display has to have retained the legend ID, which only the graph does. The palette reads that; the legend keeps the broader one. Co-Authored-By: Claude Opus 5 --- .../display-item-format-control.test.tsx | 2 +- .../inspector/display-item-format-control.tsx | 2 +- .../inspector/legend-color-controls.test.tsx | 26 +++++++- .../inspector/legend-color-controls.tsx | 6 +- ...a-configuration-legend-operability.test.ts | 62 +++++++++++++++++++ .../models/data-configuration-model.ts | 13 ++++ .../graph-data-configuration-model.test.ts | 19 ++++++ 7 files changed, 122 insertions(+), 8 deletions(-) create mode 100644 v3/src/components/data-display/models/data-configuration-legend-operability.test.ts diff --git a/v3/src/components/data-display/inspector/display-item-format-control.test.tsx b/v3/src/components/data-display/inspector/display-item-format-control.test.tsx index 1e1c2461a8..6150cf36bb 100644 --- a/v3/src/components/data-display/inspector/display-item-format-control.test.tsx +++ b/v3/src/components/data-display/inspector/display-item-format-control.test.tsx @@ -130,7 +130,7 @@ describe("DisplayItemFormatControl", () => { const desc = createMockDescription() const config = createMockDataConfig({ attributeType: jest.fn(() => "numeric"), - legendAttributeIsInoperable: true + allPointsTakeMissingColor: true }) render( + diff --git a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx index 0f4a39b286..79fa1ce1eb 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx @@ -896,7 +896,7 @@ describe("point shape controls", () => { // the graph reaches the palette with the type still resolved, which is why the rows appeared attributeType: jest.fn(() => "categorical"), categoryArrayForAttrRole: jest.fn(() => ["cat-a", "cat-b"]), - legendAttributeIsInoperable: true, + allPointsTakeMissingColor: true, ...overrides }) @@ -953,7 +953,7 @@ describe("point shape controls", () => { const desc = createMockDescription() const config = inoperableConfig({ attributeType: jest.fn(() => "numeric"), - legendAttributeIsInoperable: false + allPointsTakeMissingColor: false }) render() @@ -962,10 +962,30 @@ describe("point shape controls", () => { expect(glyph.style.getPropertyValue("--point-shape-fill")).toMatch(/^url\(#/) }) + it("keeps the color controls where the points are not drawn gray", () => { + /* + * A map layer reaches this with the assignment filtered out of attributeID, so it never + * consults the legend and its points keep the display's own color. Hiding the color control + * there would take away something that still works -- and for a polygon layer, which has no + * shape control to fall back on, it would leave the section empty. + */ + featureFlagManager.setServerConfig({ pointShapes: "on" }) + const desc = createMockDescription() + const config = createMockDataConfig({ + attributeType: jest.fn(() => undefined), + legendAttributeIsInoperable: true, + allPointsTakeMissingColor: false + }) + render() + + expect(screen.getByTestId("color-swatch-DG.Inspector.color")).toBeInTheDocument() + }) + it("leaves the rows alone when the legend is one the display can honor", () => { featureFlagManager.setServerConfig({ pointShapes: "on" }) const desc = createMockDescription() - render() expect(screen.getByTestId("color-swatch-cat-a")).toBeInTheDocument() diff --git a/v3/src/components/data-display/inspector/legend-color-controls.tsx b/v3/src/components/data-display/inspector/legend-color-controls.tsx index 31cebdac4c..63725178d1 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.tsx @@ -138,7 +138,7 @@ export const LegendColorControls = observer(function LegendColorControls( * missing-value color, so a band of the legend's colors would promise exactly what glyphColor * below exists to stop promising -- and the gradient wins, since it paints the glyph's fill. */ - const legendBandColors: string[] = attrType === "numeric" && !dataConfiguration.legendAttributeIsInoperable + const legendBandColors: string[] = attrType === "numeric" && !dataConfiguration.allPointsTakeMissingColor ? (dataConfiguration.legendNumericColorScale?.range() ?? []) : [] const legendBandStops = legendBandColors.flatMap((bandColor, i) => [ @@ -162,7 +162,7 @@ export const LegendColorControls = observer(function LegendColorControls( * them in the missing-value color whatever the point color says, so tinting the glyph with the * point color would promise something the plot does not do. */ - const glyphColor = dataConfiguration.legendAttributeIsInoperable + const glyphColor = dataConfiguration.allPointsTakeMissingColor ? missingColor : displayItemDescription.pointColor const displayShapeRow = showShape @@ -196,7 +196,7 @@ export const LegendColorControls = observer(function LegendColorControls( * the base filters out -- so without this check the palette offers colors and shapes for * categories that cannot reach the points. The legend itself says why. */ - if (dataConfiguration.legendAttributeIsInoperable) return displayShapeRow + if (dataConfiguration.allPointsTakeMissingColor) return displayShapeRow if (attrType === "categorical") { return ( diff --git a/v3/src/components/data-display/models/data-configuration-legend-operability.test.ts b/v3/src/components/data-display/models/data-configuration-legend-operability.test.ts new file mode 100644 index 0000000000..bd6545d48f --- /dev/null +++ b/v3/src/components/data-display/models/data-configuration-legend-operability.test.ts @@ -0,0 +1,62 @@ +import { Instance, types } from "@concord-consortium/mobx-state-tree" +import { DataSet, toCanonical } from "../../../models/data/data-set" +import { DataSetMetadata } from "../../../models/shared/data-set-metadata" +import { DataConfigurationModel } from "./data-configuration-model" + +/* + * The base model, which is what a map layer uses. The graph's counterpart is in + * graph-data-configuration-model.test.ts, and the two answer differently on purpose: that + * difference is why there are two views here rather than one. + * + * The configuration has to share an MST tree with the dataset, as it does in the app. A standalone + * DataConfigurationModel.create() silently keeps an undefined dataset, and then every collection + * question answers as though the data were empty. + */ +const TreeModel = types.model("Tree", { + data: DataSet, + metadata: DataSetMetadata, + config: DataConfigurationModel +}) + +describe("a legend attribute the base configuration cannot honor", () => { + let tree: Instance + + beforeEach(() => { + tree = TreeModel.create({ data: {}, metadata: {}, config: {} }) + tree.data.addAttribute({ id: "latId", name: "lat" }) + tree.data.addAttribute({ id: "legId", name: "leg" }) + tree.metadata.setData(tree.data) + tree.data.addCases(toCanonical(tree.data, [ + { __id__: "c1", lat: "shared", leg: "land" }, + { __id__: "c2", lat: "shared", leg: "water" } + ])) + tree.config.setDataset(tree.data, tree.metadata) + tree.config.setAttribute("lat", { attributeID: "latId" }) + tree.config.setAttribute("legend", { attributeID: "legId" }) + }) + + // lat moves to a parent collection, leaving the legend childmost + const makeLegendChildmost = () => tree.data.moveAttributeToNewCollection("latId") + + it("reports the assignment as inoperable, so the legend can still name it", () => { + expect(tree.config.legendAttributeIsInoperable).toBe(false) + + makeLegendChildmost() + + expect(tree.config.legendAttributeIsInoperable).toBe(true) + expect(tree.config.assignedLegendAttributeID).toBe("legId") + }) + + it("does not report the points as taking the missing color", () => { + /* + * The base filters the unusable assignment out of attributeID, so nothing consults the legend + * and the points keep the display's own color. A palette control setting that color still does + * something, and hiding it would take away a working control -- on a polygon layer, the only + * one in its section. + */ + makeLegendChildmost() + + expect(tree.config.attributeID("legend")).toBe("") + expect(tree.config.allPointsTakeMissingColor).toBe(false) + }) +}) diff --git a/v3/src/components/data-display/models/data-configuration-model.ts b/v3/src/components/data-display/models/data-configuration-model.ts index 3801c5ceef..56479f4c23 100644 --- a/v3/src/components/data-display/models/data-configuration-model.ts +++ b/v3/src/components/data-display/models/data-configuration-model.ts @@ -274,6 +274,19 @@ export const DataConfigurationModel = types return !self.isAttributeAllowedForNonAxisRole(attrID) }, + /* + * Whether every point drawn from this configuration takes the missing-value color because of + * that, which is narrower and is what a control showing or setting a point's color needs. + * + * The difference is the display. A graph goes on routing through the legend it cannot honor, + * because GraphDataConfigurationModel keeps the description the base filters out, so + * getLegendColorForCase runs and every point comes back gray. The base filters the assignment + * to "", so a map never consults the legend at all and its points keep the display's own color + * -- where hiding the color control would take away something that still works. + */ + get allPointsTakeMissingColor(): boolean { + return !!self.attributeID("legend") && this.legendAttributeIsInoperable + }, _caseHasValidValuesForDescriptions(data: IDataSet, caseID: string, descriptions: AttributeDescriptionsMapSnapshot) { return Object.entries(descriptions).every(([role, {attributeID}]) => { diff --git a/v3/src/components/graph/models/graph-data-configuration-model.test.ts b/v3/src/components/graph/models/graph-data-configuration-model.test.ts index ce14ef9304..1305f4e9cc 100644 --- a/v3/src/components/graph/models/graph-data-configuration-model.test.ts +++ b/v3/src/components/graph/models/graph-data-configuration-model.test.ts @@ -1211,6 +1211,25 @@ describe("DataConfigurationModel legend point shapes", () => { expect(tree.config.legendAttributeIsInoperable).toBe(false) }) + it("says the points take the missing color, because a graph still routes through the legend", () => { + /* + * The narrower of the two views. GraphDataConfigurationModel keeps the description the base + * filters out, so attributeID still answers and getLegendColorForCase runs and grays every + * point -- which is what makes hiding the palette's color controls correct here. + */ + makeLegendChildmost() + + expect(tree.config.attributeID("legend")).toBe("legId") + expect(tree.config.allPointsTakeMissingColor).toBe(true) + }) + + it("says they do not once the legend is honorable again", () => { + makeLegendChildmost() + tree.config.setAttribute("x", { attributeID: "legId" }) + + expect(tree.config.allPointsTakeMissingColor).toBe(false) + }) + it("does not report a deleted attribute as inoperable", () => { /* * Deleting an attribute leaves its ID behind in the legend description (see the todo in From 848af769fe3250e865cafe970d265aeae7d330df Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 14 Sep 2026 16:06:19 -0700 Subject: [PATCH 5/8] CODAP-1530: show the legend for a map layer it cannot honor MultiLegend filtered layers on attributeID("legend"), which the base configuration returns "" for when the assignment cannot be honored -- so a map layer was dropped before Legend ran, and the message, the attribute's name and the remove action never appeared there. Only the graph was getting them, contrary to what the previous commit message claimed. The filter is now a named predicate that also admits such a layer, with the visibility test unchanged. Co-Authored-By: Claude Opus 5 --- .../components/legend/multi-legend.test.ts | 41 +++++++++++++++++++ .../components/legend/multi-legend.tsx | 23 ++++++++--- 2 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 v3/src/components/data-display/components/legend/multi-legend.test.ts diff --git a/v3/src/components/data-display/components/legend/multi-legend.test.ts b/v3/src/components/data-display/components/legend/multi-legend.test.ts new file mode 100644 index 0000000000..14e6a8b94f --- /dev/null +++ b/v3/src/components/data-display/components/legend/multi-legend.test.ts @@ -0,0 +1,41 @@ +import { IBaseLayerModel } from "../../models/base-data-display-content-model" +import { layerHasLegendToShow } from "./multi-legend" + +const layer = ( + { legendID = "", inoperable = false, isVisible = true } = {} +) => ({ + id: "layer-1", + layerIndex: 0, + isVisible, + dataConfiguration: { + attributeID: () => legendID, + legendAttributeIsInoperable: inoperable + } +} as unknown as IBaseLayerModel) + +describe("layerHasLegendToShow", () => { + it("shows a layer with a legend attribute", () => { + expect(layerHasLegendToShow(layer({ legendID: "legId" }))).toBe(true) + }) + + it("skips a layer with no legend attribute", () => { + expect(layerHasLegendToShow(layer())).toBe(false) + }) + + it("skips a hidden layer", () => { + expect(layerHasLegendToShow(layer({ legendID: "legId", isVisible: false }))).toBe(false) + }) + + it("shows a layer whose legend attribute cannot be honored", () => { + /* + * The base configuration reports "" for an assignment it cannot honor, so this layer would + * otherwise be dropped here -- before Legend could render the label, the remove action, and the + * message explaining why the points are not colored by it. + */ + expect(layerHasLegendToShow(layer({ inoperable: true }))).toBe(true) + }) + + it("still skips that layer when it is hidden", () => { + expect(layerHasLegendToShow(layer({ inoperable: true, isVisible: false }))).toBe(false) + }) +}) diff --git a/v3/src/components/data-display/components/legend/multi-legend.tsx b/v3/src/components/data-display/components/legend/multi-legend.tsx index 6001b6e2ac..337ea5c8b9 100644 --- a/v3/src/components/data-display/components/legend/multi-legend.tsx +++ b/v3/src/components/data-display/components/legend/multi-legend.tsx @@ -8,11 +8,28 @@ import {useInstanceIdContext} from "../../../../hooks/use-instance-id-context" import {IDataSet} from "../../../../models/data/data-set" import {GraphPlace} from "../../../axis-graph-shared" import {GraphAttrRole} from "../../data-display-types" +import {IBaseLayerModel} from "../../models/base-data-display-content-model" import {DataConfigurationContext} from "../../hooks/use-data-configuration-context" import {useDataDisplayLayout} from "../../hooks/use-data-display-layout" import {DroppableSvg} from "../droppable-svg" import {Legend} from "./legend" +/* + * Whether a layer should be given a legend area. + * + * Graphs have one layer and it is always visible. Maps have several, and only the visible ones get + * a legend -- with one addition: a layer whose assigned legend attribute this display cannot honor + * counts as having one. Its `attributeID` reads "" in that state, because the base configuration + * filters an unusable assignment out, so testing that alone drops the layer before Legend runs and + * leaves the attribute the user assigned invisible and unremovable. That is the situation Legend + * now renders a message for, and it can only do so if the layer gets here. + */ +export function layerHasLegendToShow(layer: IBaseLayerModel): boolean { + const { dataConfiguration } = layer + return !!(dataConfiguration.attributeID("legend") || dataConfiguration.legendAttributeIsInoperable) && + layer.isVisible +} + interface IMultiLegendProps { divElt: HTMLDivElement | null onDropAttribute: (place: GraphPlace, dataSet: IDataSet, attrId: string) => void @@ -32,11 +49,7 @@ export const MultiLegend = observer(function MultiLegend({divElt, onDropAttribut extentsRef = useRef([] as number[]) const legendBoundsTop = layout?.computedBounds?.legend?.top ?? 0 - // Graphs have only one layer and it's always visible. Maps have multiple layers and we only want to display legends - // for map layers that are visible. - const layersWithLegendsArray = Array.from(dataDisplayModel.layers).filter(layer => - layer.dataConfiguration.attributeID('legend') && layer.isVisible - ) + const layersWithLegendsArray = Array.from(dataDisplayModel.layers).filter(layerHasLegendToShow) const handleIsActive = (active: Active) => { const {dataSet, attributeId: droppedAttrId} = getDragAttributeInfo(active) || {} return isDropAllowed('legend', dataSet, droppedAttrId) From dce56b35d627eee7359780118899e270572da98e Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 14 Sep 2026 17:02:21 -0700 Subject: [PATCH 6/8] CODAP-1530: make the map agree with the graph about an unusable legend The map drew normally colored points under a legend saying the attribute could not distinguish them, which is the map ignoring the assignment rather than acknowledging it. Both displays now answer the same way. getLegendColorForCase decides the inoperable case before the checks that read attributeID, since that is what differs between them: the base configuration filters an unusable assignment out, a graph's override keeps it. stylePoint follows, so a selection change repaints map points in the missing-value color rather than sending them down the no-legend path. That also removes the reason allPointsTakeMissingColor existed. Every consumer is back on legendAttributeIsInoperable, and the palette drops the color control on maps too, where it now sets something nothing reads. setLegendAttribute takes an attribute more childmost than a layer's position attributes instead of discarding it silently. Refusing never covered the case, since moving a position attribute to a parent collection reaches the same state after the assignment is made. Candidates sort by distance from the legend's collection, so the closest layer still wins. Polygon layers deliberately keep their own color; the reasoning is recorded at the call site. Co-Authored-By: Claude Opus 5 --- .../data-display/data-display-utils.ts | 12 ++++++-- .../display-item-format-control.test.tsx | 2 +- .../inspector/display-item-format-control.tsx | 2 +- .../inspector/legend-color-controls.test.tsx | 21 +++++++------- .../inspector/legend-color-controls.tsx | 6 ++-- ...a-configuration-legend-operability.test.ts | 13 +++++---- .../models/data-configuration-model.ts | 22 ++++++-------- .../graph-data-configuration-model.test.ts | 18 +++--------- .../map/components/map-polygon-layer.tsx | 6 ++++ .../map/models/map-content-model.test.ts | 29 +++++++++++++++++++ .../map/models/map-content-model.ts | 17 +++++++---- 11 files changed, 92 insertions(+), 56 deletions(-) diff --git a/v3/src/components/data-display/data-display-utils.ts b/v3/src/components/data-display/data-display-utils.ts index 440c856d2c..6487b647eb 100644 --- a/v3/src/components/data-display/data-display-utils.ts +++ b/v3/src/components/data-display/data-display-utils.ts @@ -152,6 +152,14 @@ export function setPointSelection( pointColor, pointStrokeColor, pointShape, getPointColorAtIndex } = props const dataset = dataConfiguration.dataset const legendID = dataConfiguration.attributeID('legend') + /* + * A legend the display cannot honor counts as having one here. Its ID reads "" on a map, because + * the base configuration filters an unusable assignment out, so testing the ID alone would send + * map points down the no-legend path and paint them the display's color -- while the map's own + * refresh painted them the missing-value color, and the legend said the attribute cannot + * distinguish them. The two paths have to agree. + */ + const hasLegendInEffect = !!legendID || dataConfiguration.legendAttributeIsInoperable if (!renderer) { return } @@ -160,13 +168,13 @@ export function setPointSelection( const isSelected = !!dataset?.isCaseSelected(caseID) // Determine fill color based on legend or plotNum; no-legend selected points override to blue below let fill: string - if (legendID) { + if (hasLegendInEffect) { fill = dataConfiguration?.getLegendColorForCase(caseID) } else { fill = plotNum && getPointColorAtIndex ? getPointColorAtIndex(plotNum) : pointColor } // When there's no legend, use blue fill for selection instead of a colored stroke - const useSelectionFill = isSelected && !legendID + const useSelectionFill = isSelected && !hasLegendInEffect const style: Partial = { shape: dataConfiguration.getLegendShapeForCase(caseID, pointShape), fill: useSelectionFill ? defaultSelectedColor : fill, diff --git a/v3/src/components/data-display/inspector/display-item-format-control.test.tsx b/v3/src/components/data-display/inspector/display-item-format-control.test.tsx index 6150cf36bb..1e1c2461a8 100644 --- a/v3/src/components/data-display/inspector/display-item-format-control.test.tsx +++ b/v3/src/components/data-display/inspector/display-item-format-control.test.tsx @@ -130,7 +130,7 @@ describe("DisplayItemFormatControl", () => { const desc = createMockDescription() const config = createMockDataConfig({ attributeType: jest.fn(() => "numeric"), - allPointsTakeMissingColor: true + legendAttributeIsInoperable: true }) render( + diff --git a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx index 79fa1ce1eb..56f816089f 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx @@ -896,7 +896,7 @@ describe("point shape controls", () => { // the graph reaches the palette with the type still resolved, which is why the rows appeared attributeType: jest.fn(() => "categorical"), categoryArrayForAttrRole: jest.fn(() => ["cat-a", "cat-b"]), - allPointsTakeMissingColor: true, + legendAttributeIsInoperable: true, ...overrides }) @@ -953,7 +953,7 @@ describe("point shape controls", () => { const desc = createMockDescription() const config = inoperableConfig({ attributeType: jest.fn(() => "numeric"), - allPointsTakeMissingColor: false + legendAttributeIsInoperable: false }) render() @@ -962,30 +962,29 @@ describe("point shape controls", () => { expect(glyph.style.getPropertyValue("--point-shape-fill")).toMatch(/^url\(#/) }) - it("keeps the color controls where the points are not drawn gray", () => { + it("drops the color control for a display with no legend type resolved, as a map has", () => { /* - * A map layer reaches this with the assignment filtered out of attributeID, so it never - * consults the legend and its points keep the display's own color. Hiding the color control - * there would take away something that still works -- and for a polygon layer, which has no - * shape control to fall back on, it would leave the section empty. + * A map reaches this with attributeType undefined, because the base configuration filters the + * unusable assignment out -- but its points are drawn in the missing-value color just as a + * graph's are, so a color control here would set something nothing reads. */ featureFlagManager.setServerConfig({ pointShapes: "on" }) const desc = createMockDescription() const config = createMockDataConfig({ attributeType: jest.fn(() => undefined), - legendAttributeIsInoperable: true, - allPointsTakeMissingColor: false + legendAttributeIsInoperable: true }) render() - expect(screen.getByTestId("color-swatch-DG.Inspector.color")).toBeInTheDocument() + expect(screen.queryByTestId("color-swatch-DG.Inspector.color")).not.toBeInTheDocument() + expect(screen.getByTestId("point-shape-select")).toBeInTheDocument() }) it("leaves the rows alone when the legend is one the display can honor", () => { featureFlagManager.setServerConfig({ pointShapes: "on" }) const desc = createMockDescription() - render() expect(screen.getByTestId("color-swatch-cat-a")).toBeInTheDocument() diff --git a/v3/src/components/data-display/inspector/legend-color-controls.tsx b/v3/src/components/data-display/inspector/legend-color-controls.tsx index 63725178d1..31cebdac4c 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.tsx @@ -138,7 +138,7 @@ export const LegendColorControls = observer(function LegendColorControls( * missing-value color, so a band of the legend's colors would promise exactly what glyphColor * below exists to stop promising -- and the gradient wins, since it paints the glyph's fill. */ - const legendBandColors: string[] = attrType === "numeric" && !dataConfiguration.allPointsTakeMissingColor + const legendBandColors: string[] = attrType === "numeric" && !dataConfiguration.legendAttributeIsInoperable ? (dataConfiguration.legendNumericColorScale?.range() ?? []) : [] const legendBandStops = legendBandColors.flatMap((bandColor, i) => [ @@ -162,7 +162,7 @@ export const LegendColorControls = observer(function LegendColorControls( * them in the missing-value color whatever the point color says, so tinting the glyph with the * point color would promise something the plot does not do. */ - const glyphColor = dataConfiguration.allPointsTakeMissingColor + const glyphColor = dataConfiguration.legendAttributeIsInoperable ? missingColor : displayItemDescription.pointColor const displayShapeRow = showShape @@ -196,7 +196,7 @@ export const LegendColorControls = observer(function LegendColorControls( * the base filters out -- so without this check the palette offers colors and shapes for * categories that cannot reach the points. The legend itself says why. */ - if (dataConfiguration.allPointsTakeMissingColor) return displayShapeRow + if (dataConfiguration.legendAttributeIsInoperable) return displayShapeRow if (attrType === "categorical") { return ( diff --git a/v3/src/components/data-display/models/data-configuration-legend-operability.test.ts b/v3/src/components/data-display/models/data-configuration-legend-operability.test.ts index bd6545d48f..0b28d7d646 100644 --- a/v3/src/components/data-display/models/data-configuration-legend-operability.test.ts +++ b/v3/src/components/data-display/models/data-configuration-legend-operability.test.ts @@ -1,5 +1,6 @@ import { Instance, types } from "@concord-consortium/mobx-state-tree" import { DataSet, toCanonical } from "../../../models/data/data-set" +import { missingColor } from "../../../utilities/color-utils" import { DataSetMetadata } from "../../../models/shared/data-set-metadata" import { DataConfigurationModel } from "./data-configuration-model" @@ -47,16 +48,16 @@ describe("a legend attribute the base configuration cannot honor", () => { expect(tree.config.assignedLegendAttributeID).toBe("legId") }) - it("does not report the points as taking the missing color", () => { + it("draws its points in the missing-value color, as a graph does", () => { /* - * The base filters the unusable assignment out of attributeID, so nothing consults the legend - * and the points keep the display's own color. A palette control setting that color still does - * something, and hiding it would take away a working control -- on a polygon layer, the only - * one in its section. + * The base filters the unusable assignment out of attributeID, so the checks inside + * getLegendColorForCase that read it would fall through and hand back nothing -- and the map + * would draw normally colored points under a legend saying the attribute cannot distinguish + * them. The inoperable state is answered ahead of those checks so both displays agree. */ makeLegendChildmost() expect(tree.config.attributeID("legend")).toBe("") - expect(tree.config.allPointsTakeMissingColor).toBe(false) + expect(tree.config.getLegendColorForCase("c1")).toBe(missingColor) }) }) diff --git a/v3/src/components/data-display/models/data-configuration-model.ts b/v3/src/components/data-display/models/data-configuration-model.ts index 56479f4c23..37d5406344 100644 --- a/v3/src/components/data-display/models/data-configuration-model.ts +++ b/v3/src/components/data-display/models/data-configuration-model.ts @@ -274,19 +274,6 @@ export const DataConfigurationModel = types return !self.isAttributeAllowedForNonAxisRole(attrID) }, - /* - * Whether every point drawn from this configuration takes the missing-value color because of - * that, which is narrower and is what a control showing or setting a point's color needs. - * - * The difference is the display. A graph goes on routing through the legend it cannot honor, - * because GraphDataConfigurationModel keeps the description the base filters out, so - * getLegendColorForCase runs and every point comes back gray. The base filters the assignment - * to "", so a map never consults the legend at all and its points keep the display's own color - * -- where hiding the color control would take away something that still works. - */ - get allPointsTakeMissingColor(): boolean { - return !!self.attributeID("legend") && this.legendAttributeIsInoperable - }, _caseHasValidValuesForDescriptions(data: IDataSet, caseID: string, descriptions: AttributeDescriptionsMapSnapshot) { return Object.entries(descriptions).every(([role, {attributeID}]) => { @@ -982,6 +969,15 @@ export const DataConfigurationModel = types } }, getLegendColorForCase(id: string, colorIfMissing = missingColor): string { + /* + * Answered before the checks below, which read attributeID and so answer differently by + * display: the base filters an unusable assignment out, a graph's override keeps it. Without + * this a map would fall through to its own point color and draw normally colored points + * under a legend saying the attribute cannot distinguish them. + */ + if (id && self.legendAttributeIsInoperable) { + return colorIfMissing + } const legendID = self.attributeID('legend') // todo: When user deletes we are not currently deleting the legend attribute ID. But we should. const legendAttribute = self.dataset?.getAttribute(legendID) diff --git a/v3/src/components/graph/models/graph-data-configuration-model.test.ts b/v3/src/components/graph/models/graph-data-configuration-model.test.ts index 1305f4e9cc..bd35db18cf 100644 --- a/v3/src/components/graph/models/graph-data-configuration-model.test.ts +++ b/v3/src/components/graph/models/graph-data-configuration-model.test.ts @@ -1211,23 +1211,13 @@ describe("DataConfigurationModel legend point shapes", () => { expect(tree.config.legendAttributeIsInoperable).toBe(false) }) - it("says the points take the missing color, because a graph still routes through the legend", () => { - /* - * The narrower of the two views. GraphDataConfigurationModel keeps the description the base - * filters out, so attributeID still answers and getLegendColorForCase runs and grays every - * point -- which is what makes hiding the palette's color controls correct here. - */ + it("draws its points in the missing-value color", () => { + // the base configuration reaches the same answer by a different route; see + // data-configuration-legend-operability.test.ts makeLegendChildmost() expect(tree.config.attributeID("legend")).toBe("legId") - expect(tree.config.allPointsTakeMissingColor).toBe(true) - }) - - it("says they do not once the legend is honorable again", () => { - makeLegendChildmost() - tree.config.setAttribute("x", { attributeID: "legId" }) - - expect(tree.config.allPointsTakeMissingColor).toBe(false) + expect(tree.config.getLegendColorForCase(tree.data.items[0].__id__)).toBe(missingColor) }) it("does not report a deleted attribute as inoperable", () => { diff --git a/v3/src/components/map/components/map-polygon-layer.tsx b/v3/src/components/map/components/map-polygon-layer.tsx index 3d257673ee..009e22231a 100644 --- a/v3/src/components/map/components/map-polygon-layer.tsx +++ b/v3/src/components/map/components/map-polygon-layer.tsx @@ -42,6 +42,12 @@ export const MapPolygonLayer = function MapPolygonLayer(props: { if (!dataset || !isAlive(mapLayerModel)) return const selectedCases = dataConfiguration.selection, + /* + * A legend the layer cannot honor does not count as a legend here, where on a point layer it + * does. A map carries one legend with no way to assign one per layer, so it reads as + * belonging to the points, and boundaries keep their own color rather than going gray with + * them. Gray points on gray boundaries would say less than gray points on colored ones. + */ hasLegend = !!dataConfiguration.attributeID('legend') Object.values(mapLayerModel.features).forEach((feature) => { const diff --git a/v3/src/components/map/models/map-content-model.test.ts b/v3/src/components/map/models/map-content-model.test.ts index 71f7283f8a..c9b35d8919 100644 --- a/v3/src/components/map/models/map-content-model.test.ts +++ b/v3/src/components/map/models/map-content-model.test.ts @@ -330,6 +330,35 @@ describe("MapContentModel", () => { }) }) + describe("setLegendAttribute", () => { + it("takes an attribute more childmost than the layer's position attributes", async () => { + /* + * The layer cannot color by it -- each point stands for several of its values -- but it takes + * it and says so, as a graph does. Discarding it here would be silent, and would not spare + * the user the state anyway: moving lat to a parent collection reaches it after the fact. + */ + const dataSet = DataSet.create({ name: "points" }) + dataSet.addAttribute({ id: "lat", name: "Latitude" }) + dataSet.addAttribute({ id: "long", name: "Longitude" }) + dataSet.addAttribute({ id: "leg", name: "Habitat" }) + const sharedDataSet = SharedDataSet.create() + sharedDataSet.setDataSet(dataSet) + sharedModelManager.addSharedModel(sharedDataSet) + sharedModelManager.addSharedModel(DataSetMetadata.create({ data: dataSet.id })) + await new Promise(resolve => setTimeout(resolve, 0)) + + // lat and long move to a parent collection, leaving the legend attribute childmost + dataSet.moveAttributeToNewCollection("lat") + dataSet.moveAttribute("long", { collection: dataSet.collections[0].id }) + + mapContent.setLegendAttribute(dataSet.id, "leg") + + const layer = mapContent.layers[0] + expect(layer.dataConfiguration.assignedLegendAttributeID).toBe("leg") + expect(layer.dataConfiguration.legendAttributeIsInoperable).toBe(true) + }) + }) + describe("drawsShapedItemsFor", () => { it("says a point layer draws shaped items", async () => { const dataSet = DataSet.create({ name: "points" }) diff --git a/v3/src/components/map/models/map-content-model.ts b/v3/src/components/map/models/map-content-model.ts index b0e85260a6..4acf9680a8 100644 --- a/v3/src/components/map/models/map-content-model.ts +++ b/v3/src/components/map/models/map-content-model.ts @@ -207,15 +207,22 @@ export const MapContentModel = DataDisplayContentModel const legendDataset = getDataSetFromId(self, datasetID) const legendCollectionIndex = getCollectionIndex(legendDataset!, attributeID) if (!legendDataset || legendCollectionIndex < 0) return + /* + * Every visible layer on this dataset is a candidate, including one whose GIS attribute is + * more childmost than the legend -- an assignment that layer cannot honor. It takes the + * attribute anyway and says so, the way a graph does: the legend explains that the attribute + * cannot distinguish the points, and the points draw in the missing-value color. Declining + * would not spare the user the state, since moving a position attribute to a parent + * collection reaches it after the assignment is made. + * + * Sorted by distance from the legend's collection, so the closest layer to it wins. + */ const candidateLayers = self.layers.slice() .filter(layer => layerIsMapLayerAndIsVisible(layer) && layer.data?.id === datasetID) - .filter(layer => { - const gisAttrCollectionIndex = getGisCollectionIndex(layer as IMapLayerModel) - return gisAttrCollectionIndex >= legendCollectionIndex - }).sort((layerA, layerB) => { + .sort((layerA, layerB) => { const aIndex = getGisCollectionIndex(layerA as IMapLayerModel), bIndex = getGisCollectionIndex(layerB as IMapLayerModel) - return aIndex - bIndex + return Math.abs(aIndex - legendCollectionIndex) - Math.abs(bIndex - legendCollectionIndex) }) if (candidateLayers.length > 0) { candidateLayers[0].dataConfiguration.setAttribute('legend', {attributeID, type}) From c1bcfb71be2a82477094e315a3f47481a558e05e Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 14 Sep 2026 17:32:40 -0700 Subject: [PATCH 7/8] CODAP-1530: address Copilot review of the map changes A heatmap needs a legend attribute that resolves per case, and the layer drew nothing at all without one: the heatmap gate required the attribute and the point gate required displayType to say "points". It now draws points when it cannot draw a heatmap, and the visibility reaction reads the legend attribute so a legend becoming unusable re-fires it -- refreshPoints cannot, since it returns early while the renderer is hidden. The legend's attribute menu resolved the attribute itself, reading "" for one the display cannot honor and offering no way to remove it. It takes the assigned ID as an override, so the map legend is removable as the graph's is. setLegendAttribute ranks a layer that can honor the attribute above one that cannot, and uses closeness only to choose within a rank. Closeness alone could hand the legend to a nearer layer that cannot use it while a usable one waited. Co-Authored-By: Claude Opus 5 --- .../legend/legend-attribute-label.tsx | 3 ++ .../map/components/map-point-layer.tsx | 21 +++++++++--- .../map/models/map-content-model.test.ts | 33 +++++++++++++++++++ .../map/models/map-content-model.ts | 7 +++- 4 files changed, 59 insertions(+), 5 deletions(-) diff --git a/v3/src/components/data-display/components/legend/legend-attribute-label.tsx b/v3/src/components/data-display/components/legend/legend-attribute-label.tsx index b039febbee..19f29299e5 100644 --- a/v3/src/components/data-display/components/legend/legend-attribute-label.tsx +++ b/v3/src/components/data-display/components/legend/legend-attribute-label.tsx @@ -114,6 +114,9 @@ export const LegendAttributeLabel = onChangeAttribute={onChangeAttribute} onRemoveAttribute={handleRemoveAttribute} onTreatAttributeAs={handleTreatAttributeAs} + // The menu resolves the attribute itself otherwise, and reads "" for one this display + // cannot honor -- which reads as no attribute, so it offers no way to remove it. + attrIdOverride={dataConfiguration.assignedLegendAttributeID} /> ) } diff --git a/v3/src/components/map/components/map-point-layer.tsx b/v3/src/components/map/components/map-point-layer.tsx index ab9a524ab9..cffe55faab 100644 --- a/v3/src/components/map/components/map-point-layer.tsx +++ b/v3/src/components/map/components/map-point-layer.tsx @@ -140,7 +140,13 @@ export const MapPointLayer = observer(function MapPointLayer({mapLayerModel, lay // Manage the heatmap const { isVisible: layerIsVisible, pointsAreVisible, displayType } = mapLayerModel - const displayHeatmap = displayType === "heatmap" && pointsAreVisible && layerIsVisible && legendAttributeId + /* + * A heatmap weights cases by the legend attribute, so it needs one that resolves per case. Where + * it does not -- no legend, or one this layer cannot honor -- the layer draws points instead of + * drawing nothing, since displayType alone would leave it blank. + */ + const canDrawHeatmap = !!legendAttributeId + const displayHeatmap = displayType === "heatmap" && canDrawHeatmap && pointsAreVisible && layerIsVisible // Since the canvas is only rendered when the heatmap is visible, // we need to initialize simpleheat with it whenever displayHeatmap becomes true. useEffect(() => { @@ -311,7 +317,7 @@ export const MapPointLayer = observer(function MapPointLayer({mapLayerModel, lay }, caseIdsToUpdate) }, [pointDescription, mapLayerModel, dataConfiguration, renderer]) - const displayPoints = displayType === "points" && pointsAreVisible && layerIsVisible + const displayPoints = (displayType === "points" || !canDrawHeatmap) && pointsAreVisible && layerIsVisible const refreshPoints = useDebouncedCallback(async (selectedOnly: boolean) => { const mapBounds = leafletMap.getBounds() const west = mapBounds.getWest() @@ -484,7 +490,14 @@ export const MapPointLayer = observer(function MapPointLayer({mapLayerModel, lay useEffect(function respondToLayerVisibilityChange() { return mstReaction(() => { return { - reactionDisplayPoints: mapLayerModel.displayType === "points", + /* + * Matches displayPoints above: a layer set to a heatmap it cannot draw shows points + * instead. The legend attribute is read here rather than captured outside, so that a + * legend becoming unusable re-fires this and makes the renderer visible again -- + * refreshPoints cannot, since it returns early while the renderer is hidden. + */ + reactionDisplayPoints: mapLayerModel.displayType === "points" || + !dataConfiguration.attributeID('legend'), reactionLayerIsVisible: mapLayerModel.isVisible, reactionPointsAreVisible: mapLayerModel.pointsAreVisible } @@ -503,7 +516,7 @@ export const MapPointLayer = observer(function MapPointLayer({mapLayerModel, lay }, {name: "MapPointLayer.respondToLayerVisibilityChange"}, mapLayerModel ) - }, [mapLayerModel, refreshHeatmap, refreshPoints, renderer]) + }, [dataConfiguration, mapLayerModel, refreshHeatmap, refreshPoints, renderer]) // respond to point properties change useEffect(function respondToPointVisualChange() { diff --git a/v3/src/components/map/models/map-content-model.test.ts b/v3/src/components/map/models/map-content-model.test.ts index c9b35d8919..ed11d7f65e 100644 --- a/v3/src/components/map/models/map-content-model.test.ts +++ b/v3/src/components/map/models/map-content-model.test.ts @@ -357,6 +357,39 @@ describe("MapContentModel", () => { expect(layer.dataConfiguration.assignedLegendAttributeID).toBe("leg") expect(layer.dataConfiguration.legendAttributeIsInoperable).toBe(true) }) + + it("prefers a layer that can honor the attribute over one that cannot", async () => { + /* + * Closeness alone is not enough: the layer nearest the legend's collection can be one that + * cannot honor it. Each moveAttributeToNewCollection appends a collection, so moving in this + * order builds [f0][boundary][legend][f3][lat, long] -- the boundary one collection above the + * legend and unable to honor it, lat two below and able to. + */ + const dataSet = DataSet.create({ name: "both" }) + ;["f0", "bnd", "leg", "f3", "lat", "long"].forEach(id => { + dataSet.addAttribute({ id, name: id, userType: id === "bnd" ? "boundary" : undefined }) + }) + const sharedDataSet = SharedDataSet.create() + sharedDataSet.setDataSet(dataSet) + sharedModelManager.addSharedModel(sharedDataSet) + sharedModelManager.addSharedModel(DataSetMetadata.create({ data: dataSet.id })) + await new Promise(resolve => setTimeout(resolve, 0)) + + ;["f0", "bnd", "leg", "f3"].forEach(id => dataSet.moveAttributeToNewCollection(id)) + const indexOf = (id: string) => + dataSet.collections.findIndex(col => !!col.getAttribute(id)) + expect(indexOf("bnd")).toBe(1) + expect(indexOf("leg")).toBe(2) + expect(indexOf("lat")).toBe(4) + + mapContent.setLegendAttribute(dataSet.id, "leg") + + // the polygon layer is nearer the legend, but cannot honor it + const pointLayer = mapContent.layers.find(l => isMapPointLayerModel(l)) + const polygonLayer = mapContent.layers.find(l => isMapPolygonLayerModel(l)) + expect(polygonLayer?.dataConfiguration.assignedLegendAttributeID).toBe("") + expect(pointLayer?.dataConfiguration.assignedLegendAttributeID).toBe("leg") + }) }) describe("drawsShapedItemsFor", () => { diff --git a/v3/src/components/map/models/map-content-model.ts b/v3/src/components/map/models/map-content-model.ts index 4acf9680a8..d7c43f1447 100644 --- a/v3/src/components/map/models/map-content-model.ts +++ b/v3/src/components/map/models/map-content-model.ts @@ -215,11 +215,16 @@ export const MapContentModel = DataDisplayContentModel * would not spare the user the state, since moving a position attribute to a parent * collection reaches it after the assignment is made. * - * Sorted by distance from the legend's collection, so the closest layer to it wins. + * A layer that can honor the attribute is preferred over one that cannot, and among equals + * the one whose collection is closest to the legend's wins. A layer that cannot honor it is + * the fallback rather than the answer. */ + const canHonor = (layer: IDataDisplayLayerModel) => + getGisCollectionIndex(layer as IMapLayerModel) >= legendCollectionIndex const candidateLayers = self.layers.slice() .filter(layer => layerIsMapLayerAndIsVisible(layer) && layer.data?.id === datasetID) .sort((layerA, layerB) => { + if (canHonor(layerA) !== canHonor(layerB)) return canHonor(layerA) ? -1 : 1 const aIndex = getGisCollectionIndex(layerA as IMapLayerModel), bIndex = getGisCollectionIndex(layerB as IMapLayerModel) return Math.abs(aIndex - legendCollectionIndex) - Math.abs(bIndex - legendCollectionIndex) From 1747755c02ab5295ea7cd76751bad46adb4e8b78 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Tue, 15 Sep 2026 09:09:59 -0700 Subject: [PATCH 8/8] CODAP-1530: address review comments A legend the display cannot honor does not reach everything it draws. Points take the missing-value color, so controls setting that color are inert and the palette drops them. Boundaries keep their own color, deliberately, so the control that sets it is live -- and dropping it left a polygon layer's section with nothing in it, since a polygon has no shape control to fall back on. The palette now asks whether the items it describes actually take the missing color rather than whether the legend is honorable. Connecting lines on a map went on taking the plot color while the points they joined were drawn gray, because the getter feeding them tested the filtered attribute ID. It accounts for the inoperable state, as stylePoint does. The message says "item" rather than "point", since a boundary layer reaches this state too. getLegendColorForCase no longer tests legendCollectionIsMoreChildmost: the inoperable check above it answers the same question first, and the two are exact opposites over the same childmost index. Its counterpart in getLegendShapeForCase stays live, which is now said there. placeCanAcceptAttributeIDDrop compares the assigned attribute rather than the filtered ID, so the map no longer offers to accept the attribute it holds. Co-Authored-By: Claude Opus 5 --- .../data-display/components/legend/legend.tsx | 10 +++--- .../inspector/legend-color-controls.test.tsx | 35 +++++++++++++++++++ .../inspector/legend-color-controls.tsx | 23 +++++++----- .../models/data-configuration-model.ts | 6 ++-- .../map/components/map-point-layer.tsx | 10 ++++-- .../map/models/map-content-model.ts | 4 ++- v3/src/utilities/translation/lang/en-US.json5 | 3 +- 7 files changed, 69 insertions(+), 22 deletions(-) diff --git a/v3/src/components/data-display/components/legend/legend.tsx b/v3/src/components/data-display/components/legend/legend.tsx index 7a183379bd..d248240104 100644 --- a/v3/src/components/data-display/components/legend/legend.tsx +++ b/v3/src/components/data-display/components/legend/legend.tsx @@ -39,13 +39,11 @@ export const Legend = observer(function Legend({ const dataConfiguration = useDataConfigurationContext(), legendID = dataConfiguration?.attributeID("legend"), legendRef = useRef() as React.RefObject - /* - * An assigned attribute this display cannot honor still gets a legend -- the label and the - * message, in place of the keys. Returning null for it hid the fact that the drop was accepted, - * and the remove action lives on the label, so there was no way back from the graph. - */ + // Show a legend when this display can use the attribute, and also when it cannot but the user + // assigned one anyway, since that case still needs the label, the remove action, and the message. const isInoperable = !!dataConfiguration?.legendAttributeIsInoperable - if (!isInoperable && !dataConfiguration?.isAttributeAllowedForNonAxisRole(legendID)) return null + const canShowLegend = isInoperable || !!dataConfiguration?.isAttributeAllowedForNonAxisRole(legendID) + if (!canShowLegend) return null const attrType = dataConfiguration?.attributeType('legend'), LegendComponent = dataConfiguration && legendComponentManager.getLegendComponent(dataConfiguration) diff --git a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx index 56f816089f..851ff6f6b2 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.test.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.test.tsx @@ -981,6 +981,41 @@ describe("point shape controls", () => { expect(screen.getByTestId("point-shape-select")).toBeInTheDocument() }) + it("keeps the color control on a polygon layer, which still draws in its own color", () => { + /* + * Boundaries deliberately ignore a legend the layer cannot honor -- see map-polygon-layer -- + * so the color that governs them is still this one. A polygon has no shape control to fall + * back on, so dropping the color control would leave the section with nothing in it. + */ + featureFlagManager.setServerConfig({ pointShapes: "on" }) + const desc = createMockDescription({ pointSizeMultiplier: -1 }) + const config = createMockDataConfig({ + attributeType: jest.fn(() => undefined), + legendAttributeIsInoperable: true + }) + render() + + expect(screen.getByTestId("color-swatch-DG.Inspector.color")).toBeInTheDocument() + }) + + it("drops the color control with the shape flag off, which is how it ships", () => { + // showShape is false without the flag, so the shape row is null and the early return hands + // back nothing -- correct for points, which are gray, but only if they really are the case + const desc = createMockDescription() + const config = createMockDataConfig({ + attributeType: jest.fn(() => "categorical"), + categoryArrayForAttrRole: jest.fn(() => ["cat-a", "cat-b"]), + legendAttributeIsInoperable: true + }) + render() + + expect(screen.queryByTestId("color-swatch-DG.Inspector.color")).not.toBeInTheDocument() + expect(screen.queryByTestId("color-swatch-cat-a")).not.toBeInTheDocument() + expect(screen.queryByTestId("point-shape-select")).not.toBeInTheDocument() + }) + it("leaves the rows alone when the legend is one the display can honor", () => { featureFlagManager.setServerConfig({ pointShapes: "on" }) const desc = createMockDescription() diff --git a/v3/src/components/data-display/inspector/legend-color-controls.tsx b/v3/src/components/data-display/inspector/legend-color-controls.tsx index 31cebdac4c..1d90197169 100644 --- a/v3/src/components/data-display/inspector/legend-color-controls.tsx +++ b/v3/src/components/data-display/inspector/legend-color-controls.tsx @@ -41,6 +41,15 @@ export const LegendColorControls = observer(function LegendColorControls( const attrType = dataConfiguration.attributeType("legend") // The map mounts these controls for its polygon layers too, and a polygon has no point to shape. const showShape = isFeatureEnabled("pointShapes") && !displayItemDescription.isPolygon + /* + * Whether a legend this display cannot honor actually reaches what it draws. It does for points, + * which take the missing-value color; it does not for boundaries, which keep their own color -- + * see map-polygon-layer, where that is deliberate. Controls setting a color are live in the + * second case and inert in the first. + */ + const itemsTakeMissingColor = + dataConfiguration.legendAttributeIsInoperable && !displayItemDescription.isPolygon + const categoriesRef = useRef() categoriesRef.current = dataConfiguration?.categoryArrayForAttrRole("legend") const metadata = dataConfiguration.metadata @@ -138,7 +147,7 @@ export const LegendColorControls = observer(function LegendColorControls( * missing-value color, so a band of the legend's colors would promise exactly what glyphColor * below exists to stop promising -- and the gradient wins, since it paints the glyph's fill. */ - const legendBandColors: string[] = attrType === "numeric" && !dataConfiguration.legendAttributeIsInoperable + const legendBandColors: string[] = attrType === "numeric" && !itemsTakeMissingColor ? (dataConfiguration.legendNumericColorScale?.range() ?? []) : [] const legendBandStops = legendBandColors.flatMap((bandColor, i) => [ @@ -157,13 +166,9 @@ export const LegendColorControls = observer(function LegendColorControls( * those, or a shape chosen before the legend was added becomes unreachable while it goes on * governing what is drawn. */ - /* - * The glyph shows what the points are actually drawn as. A legend this display cannot honor draws - * them in the missing-value color whatever the point color says, so tinting the glyph with the - * point color would promise something the plot does not do. - */ - const glyphColor = dataConfiguration.legendAttributeIsInoperable - ? missingColor : displayItemDescription.pointColor + // The glyph shows what is actually drawn, so tinting it with the point color where the points + // take the missing-value color would promise something the plot does not do. + const glyphColor = itemsTakeMissingColor ? missingColor : displayItemDescription.pointColor const displayShapeRow = showShape ? ( @@ -196,7 +201,7 @@ export const LegendColorControls = observer(function LegendColorControls( * the base filters out -- so without this check the palette offers colors and shapes for * categories that cannot reach the points. The legend itself says why. */ - if (dataConfiguration.legendAttributeIsInoperable) return displayShapeRow + if (itemsTakeMissingColor) return displayShapeRow if (attrType === "categorical") { return ( diff --git a/v3/src/components/data-display/models/data-configuration-model.ts b/v3/src/components/data-display/models/data-configuration-model.ts index 37d5406344..2c1a13e03b 100644 --- a/v3/src/components/data-display/models/data-configuration-model.ts +++ b/v3/src/components/data-display/models/data-configuration-model.ts @@ -985,9 +985,6 @@ export const DataConfigurationModel = types return '' } const legendType = self.attributeType('legend') - if (self.legendCollectionIsMoreChildmost) { - return colorIfMissing - } const legendValue = self.dataset?.getStrValue(id, legendID) if (!legendValue) { return colorIfMissing @@ -1016,6 +1013,9 @@ export const DataConfigurationModel = types if (!self.legendHasCategories) return shapeIfNoCategory + // This is the live check for shape, where the color path's equivalent is unreachable: color + // answers the inoperable case up front, and this returns on an empty legendID before it + // could. Both amount to falling back rather than speaking for a group. if (self.legendCollectionIsMoreChildmost) return shapeIfNoCategory const legendValue = self.dataset?.getStrValue(id, legendID) diff --git a/v3/src/components/map/components/map-point-layer.tsx b/v3/src/components/map/components/map-point-layer.tsx index cffe55faab..9dd262a8f2 100644 --- a/v3/src/components/map/components/map-point-layer.tsx +++ b/v3/src/components/map/components/map-point-layer.tsx @@ -133,7 +133,13 @@ export const MapPointLayer = observer(function MapPointLayer({mapLayerModel, lay // which case the additional validation via the DataSet would be unnecessary. const legendAttributeId = dataConfiguration.attributeID('legend') const legendAttribute = dataset?.getAttribute(legendAttributeId) - const getLegendColor = legendAttribute ? dataConfiguration?.getLegendColorForCase : undefined + /* + * A legend this layer cannot honor counts here too. Its ID reads "" -- the base configuration + * filters an unusable assignment out -- so testing the attribute alone leaves connecting lines + * to fall back to the plot color while the points they join are drawn in the missing-value one. + */ + const getLegendColor = legendAttribute || dataConfiguration.legendAttributeIsInoperable + ? dataConfiguration?.getLegendColorForCase : undefined const lookupLegendColor = (aCaseData: CaseData) => { return dataConfiguration.getLegendColorForCase(aCaseData.caseID) || pointDescription.pointColor } @@ -145,7 +151,7 @@ export const MapPointLayer = observer(function MapPointLayer({mapLayerModel, lay * it does not -- no legend, or one this layer cannot honor -- the layer draws points instead of * drawing nothing, since displayType alone would leave it blank. */ - const canDrawHeatmap = !!legendAttributeId + const canDrawHeatmap = !!legendAttributeId && !dataConfiguration.legendAttributeIsInoperable const displayHeatmap = displayType === "heatmap" && canDrawHeatmap && pointsAreVisible && layerIsVisible // Since the canvas is only rendered when the heatmap is visible, // we need to initialize simpleheat with it whenever displayHeatmap becomes true. diff --git a/v3/src/components/map/models/map-content-model.ts b/v3/src/components/map/models/map-content-model.ts index d7c43f1447..76b4fd3a30 100644 --- a/v3/src/components/map/models/map-content-model.ts +++ b/v3/src/components/map/models/map-content-model.ts @@ -533,7 +533,9 @@ export const MapContentModel = DataDisplayContentModel placeCanAcceptAttributeIDDrop(place: GraphPlace, dataset: IDataSet, attributeID: string | undefined) { if (dataset && attributeID) { const foundLayer = self.layers.find(layer => layer.data === dataset) - return !!foundLayer && foundLayer.dataConfiguration.attributeID('legend') !== attributeID + // Compared against the assigned attribute rather than attributeID, which reads "" for one + // the layer cannot honor -- so the map would offer to accept the attribute it already holds. + return !!foundLayer && foundLayer.dataConfiguration.assignedLegendAttributeID !== attributeID } return false }, diff --git a/v3/src/utilities/translation/lang/en-US.json5 b/v3/src/utilities/translation/lang/en-US.json5 index 6c61434c6a..d602793b57 100644 --- a/v3/src/utilities/translation/lang/en-US.json5 +++ b/v3/src/utilities/translation/lang/en-US.json5 @@ -1674,7 +1674,8 @@ // Shown where the legend keys would be, when the legend attribute belongs to a collection more // childmost than the plotted cases. The check is structural -- it does not look at whether the // values actually differ within a case -- so the wording says "can span". %@ is the attribute. - "V3.Legend.attributeNotOperable": "Each point can span multiple %@ values, so it can't be used to distinguish them.", + // Says "item" rather than "point" because a map's polygon layer can reach this state too. + "V3.Legend.attributeNotOperable": "Each item can span multiple %@ values, so it can't be used to distinguish them.", "V3.ResidualPlot.axisLabel": "Residuals", "V3.ResidualPlot.dataTip.residual": "Residual", "V3.Undo.graph.showResidualPlot": "Undo showing residual plot",