🐛 (explorer) merge configs correctly

owid · Oct 29, 2024 · 024ee62 · 024ee62
1 parent d2370af
commit 024ee62
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 47 deletions.
diff --git a/explorer/Explorer.tsx b/explorer/Explorer.tsx
@@ -350,7 +350,7 @@ export class Explorer
             reaction(
                 () => [
                     this.entityPickerMetric,
-                    this.explorerProgram.grapherConfig.tableSlug,
+                    this.explorerProgram.explorerGrapherConfig.tableSlug,
                 ],
                 () => this.updateEntityPickerTable()
             )
@@ -446,31 +446,10 @@ export class Explorer
         }
     }
 
+    // TODO: remove and add to individual methods
     @action.bound private updateGrapherFromExplorerCommon() {
         const grapher = this.grapher
         if (!grapher) return
-        const {
-            yScaleToggle,
-            yAxisMin,
-            facetYDomain,
-            relatedQuestionText,
-            relatedQuestionUrl,
-            mapTargetTime,
-        } = this.explorerProgram.grapherConfig
-
-        grapher.yAxis.canChangeScaleType = yScaleToggle
-        grapher.yAxis.min = yAxisMin
-        if (facetYDomain) {
-            grapher.yAxis.facetDomain = facetYDomain
-        }
-        if (relatedQuestionText && relatedQuestionUrl) {
-            grapher.relatedQuestions = [
-                { text: relatedQuestionText, url: relatedQuestionUrl },
-            ]
-        }
-        if (mapTargetTime) {
-            grapher.map.time = mapTargetTime
-        }
         grapher.slug = this.explorerProgram.slug
         if (!grapher.id) grapher.id = 0
     }
@@ -525,12 +504,12 @@ export class Explorer
         const grapher = this.grapher
         if (!grapher) return
 
-        const { grapherId } = this.explorerProgram.grapherConfig
+        const { grapherId } = this.explorerProgram.explorerGrapherConfig
         const grapherConfig = this.grapherConfigs.get(grapherId!) ?? {}
 
         const config: GrapherProgrammaticInterface = {
             ...grapherConfig,
-            ...this.explorerProgram.grapherConfigOnlyGrapherProps,
+            ...this.explorerProgram.grapherConfig,
             bakedGrapherURL: BAKED_GRAPHER_URL,
             dataApiUrl: DATA_API_URL,
             hideEntityControls: this.showExplorerControls,
@@ -561,7 +540,7 @@ export class Explorer
             xSlug,
             colorSlug,
             sizeSlug,
-        } = this.explorerProgram.grapherConfig
+        } = this.explorerProgram.explorerGrapherConfig
 
         const yVariableIdsList = yVariableIds
             .split(" ")
@@ -574,7 +553,7 @@ export class Explorer
 
         const config: GrapherProgrammaticInterface = {
             ...partialGrapherConfig,
-            ...this.explorerProgram.grapherConfigOnlyGrapherProps,
+            ...this.explorerProgram.grapherConfig,
             bakedGrapherURL: BAKED_GRAPHER_URL,
             dataApiUrl: DATA_API_URL,
             hideEntityControls: this.showExplorerControls,
@@ -722,10 +701,10 @@ export class Explorer
     @action.bound private updateGrapherFromExplorerUsingColumnSlugs() {
         const grapher = this.grapher
         if (!grapher) return
-        const { tableSlug } = this.explorerProgram.grapherConfig
+        const { tableSlug } = this.explorerProgram.explorerGrapherConfig
 
         const config: GrapherProgrammaticInterface = {
-            ...this.explorerProgram.grapherConfigOnlyGrapherProps,
+            ...this.explorerProgram.grapherConfig,
             bakedGrapherURL: BAKED_GRAPHER_URL,
             dataApiUrl: DATA_API_URL,
             hideEntityControls: this.showExplorerControls,
@@ -1072,7 +1051,7 @@ export class Explorer
         // so that when we start sorting by entity name we can infer that the column is a string column immediately.
         const tableSlugToLoad = this.entityPickerMetric
             ? this.getTableSlugOfColumnSlug(this.entityPickerMetric)
-            : this.explorerProgram.grapherConfig.tableSlug
+            : this.explorerProgram.explorerGrapherConfig.tableSlug
 
         this.entityPickerTableIsLoading = true
         void this.futureEntityPickerTable.set(
@@ -1108,7 +1087,8 @@ export class Explorer
 
         // In most cases, column slugs will be duplicated in the tables, e.g. entityName.
         // Prefer the current Grapher table if it contains the column slug.
-        const grapherTableSlug = this.explorerProgram.grapherConfig.tableSlug
+        const grapherTableSlug =
+            this.explorerProgram.explorerGrapherConfig.tableSlug
         if (
             this.tableSlugHasColumnSlug(
                 grapherTableSlug,

diff --git a/explorer/ExplorerProgram.ts b/explorer/ExplorerProgram.ts
@@ -18,7 +18,7 @@ import {
     PromiseCache,
     SerializedGridProgram,
     trimObject,
-    omit,
+    merge,
 } from "@ourworldindata/utils"
 import {
     CellDef,
@@ -358,29 +358,59 @@ export class ExplorerProgram extends GridProgram {
         return clone
     }
 
-    get grapherConfig(): ExplorerGrapherInterface {
+    /**
+     * Grapher config for the currently selected row including global settings.
+     * Includes all columns that are part of the GrapherGrammar.
+     */
+    get explorerGrapherConfig(): ExplorerGrapherInterface {
         const rootObject = trimAndParseObject(this.tuplesObject, GrapherGrammar)
-
-        Object.keys(rootObject).forEach((key) => {
-            if (!GrapherGrammar[key]) delete rootObject[key]
-        })
+        let config = { ...rootObject }
 
         const selectedGrapherRow = this.decisionMatrix.selectedRow
         if (selectedGrapherRow && Object.keys(selectedGrapherRow).length) {
-            return { ...rootObject, ...selectedGrapherRow }
+            config = { ...config, ...selectedGrapherRow }
         }
 
-        return rootObject
+        // remove all keys that are not part of the GrapherGrammar
+        Object.keys(config).forEach((key) => {
+            if (!GrapherGrammar[key]) delete rootObject[key]
+        })
+
+        return config
     }
 
-    get grapherConfigOnlyGrapherProps() {
-        return omit(this.grapherConfig, [
-            GrapherGrammar.yVariableIds.keyword,
-            GrapherGrammar.xVariableId.keyword,
-            GrapherGrammar.colorVariableId.keyword,
-            GrapherGrammar.sizeVariableId.keyword,
-            GrapherGrammar.mapTargetTime.keyword,
-        ])
+    /**
+     * Grapher config for the currently selected row, with explorer-specific
+     * fields translated to valid GrapherInterface fields.
+     *
+     * For example, `yAxisMin` is translated to `{yAxis: {min: ...}}`
+     */
+    get grapherConfig(): GrapherInterface {
+        const configs = []
+
+        for (const [field, value] of Object.entries(
+            this.explorerGrapherConfig
+        )) {
+            const grammarDef = GrapherGrammar[field]
+            if (grammarDef?.toValidConfig) {
+                configs.push(grammarDef.toValidConfig(value))
+            } else {
+                configs.push({ [field]: value })
+            }
+        }
+
+        const merged = merge({}, ...configs)
+
+        // TODO: turn related questions into two fields instead?
+        const { relatedQuestionUrl, relatedQuestionText } =
+            this.explorerGrapherConfig
+        if (relatedQuestionUrl && relatedQuestionText) {
+            merged.relatedQuestions = [
+                { url: relatedQuestionUrl, text: relatedQuestionText },
+            ]
+        }
+
+        return merged
     }
 
     /**

diff --git a/explorer/GrapherGrammar.ts b/explorer/GrapherGrammar.ts
@@ -54,6 +54,7 @@ export const GrapherGrammar: Grammar = {
         ...IndicatorIdsOrEtlPathsCellDef,
         keyword: "yVariableIds",
         description: "Variable ID(s) or ETL path(s) for the yAxis",
+        toValidConfig: () => ({}), // processed by the explorer, no need to include in the grapher config
     },
     type: {
         ...StringCellDef,
@@ -65,12 +66,14 @@ export const GrapherGrammar: Grammar = {
         ...IntegerCellDef,
         description: "ID of a legacy Grapher to load",
         keyword: "grapherId",
+        toValidConfig: (value) => ({ id: value ?? 0 }),
     },
     tableSlug: {
         ...SlugDeclarationCellDef,
         description:
             "Slug of the explorer table (i.e. csv file) to use for this row. All variables used in this row must be present in the table/file.",
         keyword: "tableSlug",
+        toValidConfig: () => ({}), // processed by the explorer, no need to include in the grapher config
     },
     hasMapTab: {
         ...BooleanCellDef,
@@ -97,6 +100,7 @@ export const GrapherGrammar: Grammar = {
         ...IndicatorIdOrEtlPathCellDef,
         keyword: "xVariableId",
         description: "Variable ID or ETL path for the xAxis",
+        toValidConfig: () => ({}), // processed by the explorer, no need to include in the grapher config
     },
     colorSlug: {
         ...SlugDeclarationCellDef,
@@ -107,6 +111,7 @@ export const GrapherGrammar: Grammar = {
         ...IndicatorIdOrEtlPathCellDef,
         keyword: "colorVariableId",
         description: "Variable ID or ETL path for the color",
+        toValidConfig: () => ({}), // processed by the explorer, no need to include in the grapher config
     },
     sizeSlug: {
         ...SlugDeclarationCellDef,
@@ -118,6 +123,7 @@ export const GrapherGrammar: Grammar = {
         keyword: "sizeVariableId",
         description:
             "Variable ID or ETL path for the size of points on scatters",
+        toValidConfig: () => ({}), // processed by the explorer, no need to include in the grapher config
     },
     tableSlugs: {
         ...SlugsDeclarationCellDef,
@@ -147,18 +153,21 @@ export const GrapherGrammar: Grammar = {
         ...BooleanCellDef,
         keyword: "yScaleToggle",
         description: "Set to 'true' if the user can change the yAxis",
+        toValidConfig: (value) => ({ yAxis: { canChangeScaleType: value } }),
     },
     yAxisMin: {
         ...NumericCellDef,
         keyword: "yAxisMin",
         description: "Set the minimum value for the yAxis",
+        toValidConfig: (value) => ({ yAxis: { min: value } }),
     },
     facetYDomain: {
         ...EnumCellDef,
         keyword: "facetYDomain",
         description:
             "Whether facets axes default to shared or independent domain",
         terminalOptions: toTerminalOptions(Object.values(FacetAxisDomain)),
+        toValidConfig: (value) => ({ yAxis: { facetDomain: value } }),
     },
     selectedFacetStrategy: {
         ...EnumCellDef,
@@ -242,17 +251,20 @@ export const GrapherGrammar: Grammar = {
         keyword: "relatedQuestionText",
         description:
             "The text used for the related question (at the very bottom of the chart)",
+        toValidConfig: (value) => ({ relatedQuestions: [{ text: value }] }),
     },
     relatedQuestionUrl: {
         ...UrlCellDef,
         keyword: "relatedQuestionUrl",
         description: "The link of the related question text",
+        toValidConfig: (value) => ({ relatedQuestions: [{ url: value }] }),
     },
     mapTargetTime: {
         ...IntegerCellDef,
         keyword: "mapTargetTime",
         description:
             "Set the 'target time' for the map chart. This is the year that will be shown by default in the map chart.",
+        toValidConfig: (value) => ({ map: { time: value } }),
     },
     missingDataStrategy: {
         ...EnumCellDef,

diff --git a/gridLang/GridLangConstants.ts b/gridLang/GridLangConstants.ts
@@ -28,6 +28,7 @@ export interface CellDef {
     positionalCellDefs?: readonly CellDef[] // Additional cell types as positional arguments.
     isHorizontalList?: boolean // a list type, such as "colors\tred\torange\tgreen"
     parse?: (value: any) => any
+    toValidConfig?: (value: any) => any // map to a partial config that is a valid GrapherInterface
 }
 
 export interface ParsedCell {