Merge pull request #941 from concord-consortium/186164083-fix-adornme…

…nts-not-always-rendered-for-all-subplots fix: Adornments not always rendered for all subplots (PT-186164083)
concord-consortium · Oct 13, 2023 · 52e785a · 52e785a
2 parents 1240948 + 9dca574
commit 52e785a
Show file tree

Hide file tree

Showing 7 changed files with 28 additions and 20 deletions.
diff --git a/v3/src/components/graph/adornments/adornment-models.test.ts b/v3/src/components/graph/adornments/adornment-models.test.ts
@@ -52,7 +52,7 @@ describe("AdornmentModel", () => {
       rightCats: ["new", "used"]
     }
     const adornment = AdornmentModel.create({type: "Movable Line"})
-    const cellKey = adornment.setCellKey(options, 0)
+    const cellKey = adornment.cellKey(options, 0)
     expect(cellKey).toEqual({abc123: "pizza", def456: "red", ghi789: "small", jkl012: "new"})
   })
   it("will create an instance key value from given category values", () => {

diff --git a/v3/src/components/graph/adornments/adornment-models.ts b/v3/src/components/graph/adornments/adornment-models.ts
@@ -63,45 +63,53 @@ export const AdornmentModel = types.model("AdornmentModel", {
     },
     get isUnivariateMeasure() {
       return false
-    }
-  }))
-  .actions(self => ({
-    setVisibility(isVisible: boolean) {
-      self.isVisible = isVisible
     },
-    updateCategories(options: IUpdateCategoriesOptions) {
-      // derived models should override to update their models when categories change
-    },
-    setCellKey(options: IUpdateCategoriesOptions, index: number) {
+    cellKey(options: IUpdateCategoriesOptions, index: number) {
       const { xAttrId, xCats, yAttrId, yCats, topAttrId, topCats, rightAttrId, rightCats } = options
       const topCatCount = topCats.length || 1
       const rightCatCount = rightCats.length || 1
-      const xCatCount = xCats.length || 1
       const yCatCount = yCats.length || 1
+      const xCatCount = xCats.length || 1
       const columnCount = topCatCount * xCatCount
       const rowCount = rightCatCount * yCatCount
       const cellKey: Record<string, string> = {}
+
+      // Determine which categories are associated with the cell's axes using the provided index value and
+      // the attributes and categories present in the graph.      
       const topIndex = xCatCount > 0
         ? Math.floor(index / xCatCount) % topCatCount
         : index % topCatCount
       const topCat = topCats[topIndex]
-      const rightIndex = yCatCount > 0
-        ? Math.floor(index / yCatCount) % rightCats.length
-        : index % rightCats.length
+      const rightIndex = topCatCount > 0 && yCatCount > 1
+        ? Math.floor(index / (topCatCount * yCatCount)) % rightCatCount
+        : yCatCount > 0
+          ? Math.floor(index / yCatCount) % rightCatCount
+          : index % rightCatCount
       const rightCat = rightCats[rightIndex]
       const yCat = topCats.length > 0
         ? yCats[Math.floor(index / columnCount) % yCatCount]
         : yCats[index % yCats.length]
       const xCat = rightCats.length > 0
         ? xCats[Math.floor(index / rowCount) % xCatCount]
         : xCats[index % xCats.length]
+
+      // Assign the categories determined above to the associated properties of the cellKey object.
       if (topAttrId) cellKey[topAttrId] = topCat
       if (rightAttrId) cellKey[rightAttrId] = rightCat
       if (yAttrId && yCats[0]) cellKey[yAttrId] = yCat
       if (xAttrId && xCats[0]) cellKey[xAttrId] = xCat
+
       return cellKey
     }
   }))
+  .actions(self => ({
+    setVisibility(isVisible: boolean) {
+      self.isVisible = isVisible
+    },
+    updateCategories(options: IUpdateCategoriesOptions) {
+      // derived models should override to update their models when categories change
+    }
+  }))
 export interface IAdornmentModel extends Instance<typeof AdornmentModel> {}
 
 export const UnknownAdornmentModel = AdornmentModel

diff --git a/v3/src/components/graph/adornments/adornments-store.test.ts b/v3/src/components/graph/adornments/adornments-store.test.ts
@@ -15,7 +15,7 @@ const mockAdornment = {
   instanceKey: () => "mock-count-adornment",
   isUnivariateMeasure: false,
   isVisible: false,
-  setCellKey: () => ({}),
+  cellKey: () => ({}),
   setVisibility: () => true,
   updateCategories: () => ({}),
   type: "Mock Adornment"

diff --git a/v3/src/components/graph/adornments/movable-line/movable-line-adornment-model.ts b/v3/src/components/graph/adornments/movable-line/movable-line-adornment-model.ts
@@ -56,7 +56,7 @@ export const MovableLineAdornmentModel = AdornmentModel
     const rowCount = rightCats?.length || 1
     const totalCount = rowCount * columnCount
     for (let i = 0; i < totalCount; ++i) {
-      const cellKey = self.setCellKey(options, i)
+      const cellKey = self.cellKey(options, i)
       const instanceKey = self.instanceKey(cellKey)
       if (!self.lines.get(instanceKey) || resetPoints) {
         self.setInitialLine(xAxis, yAxis, instanceKey)

diff --git a/v3/src/components/graph/adornments/movable-point/movable-point-adornment-model.ts b/v3/src/components/graph/adornments/movable-point/movable-point-adornment-model.ts
@@ -35,7 +35,7 @@ export const MovablePointAdornmentModel = AdornmentModel
       const rowCount = rightCats?.length || 1
       const totalCount = rowCount * columnCount
       for (let i = 0; i < totalCount; ++i) {
-        const cellKey = self.setCellKey(options, i)
+        const cellKey = self.cellKey(options, i)
         const instanceKey = self.instanceKey(cellKey)
         if (!self.points.get(instanceKey) || resetPoints) {
           self.setInitialPoint(xAxis, yAxis, instanceKey)

diff --git a/v3/src/components/graph/adornments/movable-value/movable-value-adornment-model.ts b/v3/src/components/graph/adornments/movable-value/movable-value-adornment-model.ts
@@ -130,7 +130,7 @@ export const MovableValueAdornmentModel = AdornmentModel
       self.setAxisMax(axisMax)
 
       for (let i = 0; i < totalCount; ++i) {
-        const subPlotKey = self.setCellKey(options, i)
+        const subPlotKey = self.cellKey(options, i)
         const instanceKey = self.instanceKey(subPlotKey)
         // Each array in the model's values map should have the same length as all the others. If there are no existing
         // values for the current instance key, check if there is at least one array in the map. If there is, copy those

diff --git a/v3/src/components/graph/adornments/univariate-measures/univariate-measure-adornment-model.ts b/v3/src/components/graph/adornments/univariate-measures/univariate-measure-adornment-model.ts
@@ -100,8 +100,8 @@ export const UnivariateMeasureAdornmentModel = AdornmentModel
       const totalCount = rowCount * columnCount
       const attrId = dataConfig.primaryAttributeID
       for (let i = 0; i < totalCount; ++i) {
-        const cellKey = self.setCellKey(options, i)
-        const instanceKey = self.instanceKey(cellKey) 
+        const cellKey = self.cellKey(options, i)
+        const instanceKey = self.instanceKey(cellKey)
         const value = Number(self.computeMeasureValue(attrId, cellKey, dataConfig))
         if (!self.measures.get(instanceKey) || resetPoints) {
           self.addMeasure(value, instanceKey)