diff --git a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js index 8dbf21ff86..7eeab5d3a0 100644 --- a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js +++ b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js @@ -305,6 +305,35 @@ context('XYPlot Tool Tile', function () { xyTile.getGraphDot().should('have.length', 8); }); + it("On initialization from data card containing image URLs, does not graph image URLs", () => { + beforeTest(`${Cypress.config("qaMothPlotUnitStudent5")}&mouseSensor`); + clueCanvas.addTile("datacard"); + dataCard.getTile().should("exist"); + dataCard.getAddAttributeButton().click(); + dataCard.getAttrName().eq(0).dblclick().type("Image{enter}"); + dataCard.getAddAttributeButton().click(); + dataCard.getAttrName().eq(1).dblclick().type("Name{enter}"); + dataCard.getAddAttributeButton().click(); + dataCard.getAttrName().eq(2).dblclick().type("Type{enter}"); + dataCard.getAttrValue().eq(0).click().type("https://concord.org/images/energy3d.png{enter}"); + dataCard.getAttrValue().eq(1).click().type("Energy3D{enter}"); + dataCard.getAttrValue().eq(2).click().type("download{enter}"); + dataCard.getAddCardButton().click(); + dataCard.getAttrValue().eq(0).click().type("https://concord.org/images/codap.png{enter}"); + dataCard.getAttrValue().eq(1).click().type("CODAP{enter}"); + dataCard.getAttrValue().eq(2).click().type("web app{enter}"); + dataCard.getGraphItButton().click(); + cy.wait(1000); + // Image URLs should not be graphed. + xyTile.getXAxisLabel().should("contain", "Name"); + xyTile.getYAxisLabel().should("contain", "Type"); + // If the user assigns the image URLs to an axis, "" should be used in place of the full URL values + // for the tick labels. + xyTile.clickPortalButton("Name"); + xyTile.getPortalButton().contains("Image").should("exist").click(); + cy.get("[data-testid=axis-bottom]").find("text").should("contain", ""); + }); + it("Test undo redo actions", () => { beforeTest(queryParamsMultiDataset); cy.log("Undo redo XY Plot Tile creation"); diff --git a/src/models/data/data-types.test.ts b/src/models/data/data-types.test.ts index 200c23de59..0fba5d590a 100644 --- a/src/models/data/data-types.test.ts +++ b/src/models/data/data-types.test.ts @@ -163,7 +163,7 @@ describe("data-types", () => { return [value, isImageUrl(value)]; }); expect({ testCases }).toMatchInlineSnapshot(` -"https://something.concord.org/hello.png" => false +"https://something.concord.org/hello.png" => true "ccimg://fbrtdb.concord.org/hello.png" => true "ccimg://fbrtdb.concord.org/anything.txt" => true "ccimg://not-a-cc.domain.com/hello.png" => false diff --git a/src/models/data/data-types.ts b/src/models/data/data-types.ts index 8f5a3e18cc..95d1fe7749 100644 --- a/src/models/data/data-types.ts +++ b/src/models/data/data-types.ts @@ -126,12 +126,13 @@ export function isImageUrl(val: IValueType) { return false; } - // We might need to support more advanced image detection in the future - // For now we just look for strings matching the URL for an user uploaded - // image. Like: + // Look for strings matching the URL for a user uploaded image, like: // ccimg://fbrtdb.concord.org/devclass/-NcP-LmubeWUdANUM_vO - // The ImageMap has a isImageUrl function but it will pickup pretty - // much any http URL. And I'd guess we don't want to add a dependency - // on the imageMap to this shared code. - return val.startsWith("ccimg://fbrtdb.concord.org"); + // Or that match more generic image URL strings, like: + // https://concord.org/image.png + // The ImageMap has an isImageUrl function but it will pickup pretty much any http URL. + // And we don't want to add a dependency on the imageMap to this shared code. + const ccImagePattern = /^ccimg:\/\/fbrtdb\.concord\.org/; + const imageUrlPattern = /^(https?:\/\/.*\.(gif|jpg|jpeg|png)|data:image\/(gif|jpg|jpeg|png);base64,)/i; + return ccImagePattern.test(val) || imageUrlPattern.test(val); } diff --git a/src/plugins/graph/imports/components/axis/hooks/use-axis.ts b/src/plugins/graph/imports/components/axis/hooks/use-axis.ts index d4c0d6482c..5d8b3180b2 100644 --- a/src/plugins/graph/imports/components/axis/hooks/use-axis.ts +++ b/src/plugins/graph/imports/components/axis/hooks/use-axis.ts @@ -12,6 +12,7 @@ import {maxWidthOfStringsD3} from "../../../../utilities/graph-utils"; import {useDataConfigurationContext} from "../../../../hooks/use-data-configuration-context"; import {collisionExists, getStringBounds} from "../axis-utils"; import graphVars from "../../../../components/graph.scss"; +import { isImageUrl } from "../../../../../../models/data/data-types"; export interface IUseAxis { axisModel?: IAxisModel @@ -66,7 +67,10 @@ export const useAxis = ({ repetitions = multiScale?.repetitions ?? 1, bandWidth = ((ordinalScale?.bandwidth?.()) ?? 0) / repetitions, collision = collisionExists({bandWidth, categories, centerCategoryLabels}), - maxLabelExtent = maxWidthOfStringsD3(dataConfiguration?.categoryArrayForAttrRole(attrRole) ?? []), + categoryArrayValues = dataConfiguration?.categoryArrayForAttrRole(attrRole), + // Image URLs will be replaced with the placeholder "", so do not measure the URL values + validCategoryArrayValues = categoryArrayValues?.map(cat => isImageUrl(cat) ? "" : cat), + maxLabelExtent = maxWidthOfStringsD3(validCategoryArrayValues ?? []), d3Scale = multiScale?.scale ?? (type === 'numeric' ? scaleLinear() : scaleOrdinal()); let desiredExtent = axisTitleHeight + 2 * (axisGap + kAxisLabelVerticalPadding) + kAxisTickLength + kAxisTickPadding; diff --git a/src/plugins/graph/imports/components/axis/hooks/use-sub-axis.ts b/src/plugins/graph/imports/components/axis/hooks/use-sub-axis.ts index 4800bd79a5..5773dd984d 100644 --- a/src/plugins/graph/imports/components/axis/hooks/use-sub-axis.ts +++ b/src/plugins/graph/imports/components/axis/hooks/use-sub-axis.ts @@ -12,6 +12,7 @@ import { import {DragInfo, collisionExists, computeBestNumberOfTicks, getCategoricalLabelPlacement,getCoordFunctions, IGetCoordFunctionsProps} from "../axis-utils"; import { useGraphModelContext } from "../../../../hooks/use-graph-model-context"; +import { isImageUrl } from "../../../../../../models/data/data-types"; // This function is used to style to style the axes of multiple types of plots below. // It would be better if we had functions like this to style other features consistently (tick marks, grid lines). @@ -152,7 +153,9 @@ export const useSubAxis = ({subAxisIndex, axisModel, subAxisElt, showScatterPlot const dividerLength = layout.getAxisLength(otherPlace(place)) ?? 0; const isRightCat = place === 'rightCat'; const isTop = place === 'top'; - const categories = Array.from(categorySet?.values ?? []); + const _categories = Array.from(categorySet?.values ?? []); + // Replace image URLs with placeholder string + const categories = _categories.map(cat => isImageUrl(cat) ? "" : cat); const numCategories = categories.length; const bandWidth = subAxisLength / numCategories; const collision = collisionExists({bandWidth, categories, centerCategoryLabels}); @@ -302,8 +305,11 @@ export const useSubAxis = ({subAxisIndex, axisModel, subAxisElt, showScatterPlot multiScale = layout.getAxisMultiScale(place), categorySet = multiScale?.categorySet, categories = Array.from(categorySet?.values ?? []), - categoryData: CatObject[] = categories.map((cat, index) => - ({cat, index: isVertical(place) ? categories.length - index - 1 : index})); + categoryData: CatObject[] = categories.map((cat, index) => { + // Replace image URLs with a placeholder string + const validCatValue = isImageUrl(cat) ? "" : cat; + return ({cat: validCatValue, index: isVertical(place) ? categories.length - index - 1 : index}); + }); subAxisSelectionRef.current = select(subAxisElt); const sAS = subAxisSelectionRef.current; diff --git a/src/plugins/graph/models/graph-layer-model.ts b/src/plugins/graph/models/graph-layer-model.ts index 33e87ac01a..b839edfabc 100644 --- a/src/plugins/graph/models/graph-layer-model.ts +++ b/src/plugins/graph/models/graph-layer-model.ts @@ -11,6 +11,7 @@ import { IDataSet, addCanonicalCasesToDataSet } from "../../../models/data/data- import { ISharedCaseMetadata } from "../../../models/shared/shared-case-metadata"; import { DotsElt } from "../d3-types"; import { ICaseCreation } from "../../../models/data/data-set-types"; +import { isImageUrl } from "../../../models/data/data-types"; export const GraphLayerModel = types .model('GraphLayerModel') @@ -101,10 +102,19 @@ export const GraphLayerModel = types const isValidYAttr = yAttrId && !!data?.attrFromID(yAttrId); if (!isValidXAttr && !isValidYAttr) { - this.autoAssignAttributeID("bottom", "x", data?.id ?? "", data?.attributes[0].id || ''); - if (attributeCount > 1) { - this.autoAssignAttributeID("left", "y", data?.id ?? "", data?.attributes[1].id || ''); - } + const validAttributes = data?.attributes.filter(attr => { + return data?.cases.every((c) => { + const value = data?.getValue(c.__id__, attr.id); + // Do not auto assign attributes containing image URLs. + return !(typeof value === "string" && isImageUrl(value)); + }); + }); + + const bottomAttrId = validAttributes && validAttributes.length > 0 ? validAttributes[0].id : ""; + const leftAttrId = validAttributes && validAttributes.length > 1 ? validAttributes[1].id : ""; + + this.autoAssignAttributeID("bottom", "x", data?.id ?? "", bottomAttrId); + this.autoAssignAttributeID("left", "y", data?.id ?? "", leftAttrId); } } else { console.log('autoAssign is off');