cleanup

adminSiteClient/AbstractChartEditor.ts

@@ -44,7 +44,7 @@ export abstract class AbstractChartEditor<
     @observable.ref showStaticPreview = false
     @observable.ref savedPatchConfig: GrapherInterface = {}
 
-    // parent config derived from the current chart config (not necessarily in use)
+    // parent config derived from the current chart config
     @observable.ref parentConfig: GrapherInterface | undefined = undefined
     // if inheritance is enabled, the parent config is applied to grapher
     @observable.ref isInheritanceEnabled: boolean | undefined = undefined
@@ -92,14 +92,6 @@ export abstract class AbstractChartEditor<
         return mergeGrapherConfigs(this.fullDefaultObject, this.grapher.object)
     }
 
-    /** patch config of the chart that is written to the db on save */
-    @computed get patchConfig(): GrapherInterface {
-        return diffGrapherConfigs(
-            this.fullConfig,
-            this.activeParentConfigWithDefaults ?? this.fullDefaultObject
-        )
-    }
-
     /** parent config currently applied to grapher */
     @computed get activeParentConfig(): GrapherInterface | undefined {
         return this.isInheritanceEnabled ? this.parentConfig : undefined
@@ -115,6 +107,14 @@ export abstract class AbstractChartEditor<
         )
     }
 
+    /** patch config of the chart that is written to the db on save */
+    @computed get patchConfig(): GrapherInterface {
+        return diffGrapherConfigs(
+            this.fullConfig,
+            this.activeParentConfigWithDefaults ?? this.fullDefaultObject
+        )
+    }
+
     @computed get isModified(): boolean {
         return !isEqual(
             omit(this.patchConfig, "version"),
@@ -132,15 +132,6 @@ export abstract class AbstractChartEditor<
         this.grapher.updateAuthoredVersion(config)
     }
 
-    // TODO: only works for first level at the moment
-    isPropertyInherited(property: keyof GrapherInterface): boolean {
-        if (!this.activeParentConfig) return false
-        return (
-            !Object.hasOwn(this.patchConfig, property) &&
-            Object.hasOwn(this.activeParentConfig, property)
-        )
-    }
-
     abstract get isNewGrapher(): boolean
     abstract get availableTabs(): EditorTab[]
 

adminSiteClient/AdminApp.tsx

@@ -41,7 +41,6 @@ import { BulkGrapherConfigEditorPage } from "./BulkGrapherConfigEditor.js"
 import { GdocsIndexPage, GdocsMatchProps } from "./GdocsIndexPage.js"
 import { GdocsPreviewPage } from "./GdocsPreviewPage.js"
 import { GdocsStoreProvider } from "./GdocsStore.js"
-import { IndicatorChartEditorPage } from "./IndicatorChartEditorPage.js"
 
 @observer
 class AdminErrorMessage extends React.Component<{ admin: Admin }> {
@@ -155,7 +154,7 @@ export class AdminApp extends React.Component<{
                                 path="/charts"
                                 component={ChartIndexPage}
                             />
-                            <Route
+                            {/* <Route
                                 exact
                                 path="/indicator-charts/:variableId/edit"
                                 render={({ match }) => (
@@ -165,7 +164,7 @@ export class AdminApp extends React.Component<{
                                         )}
                                     />
                                 )}
-                            />
+                            /> */}
                             <Route
                                 exact
                                 path={`/${EXPLORERS_ROUTE_FOLDER}/:slug`}

adminSiteClient/ChartEditorPage.tsx

@@ -14,7 +14,7 @@ import {
     ChartEditorManager,
     fetchMergedGrapherConfigByVariableId,
 } from "./ChartEditor.js"
-import { AdminAppContext } from "./AdminAppContext.js"
+import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js"
 import { ChartEditorView, ChartEditorViewManager } from "./ChartEditorView.js"
 
 @observer
@@ -25,14 +25,15 @@ export class ChartEditorPage
     }>
     implements ChartEditorManager, ChartEditorViewManager<ChartEditor>
 {
+    static contextType = AdminAppContext
+    context!: AdminAppContextType
+
     @observable logs: Log[] = []
     @observable references: References | undefined = undefined
     @observable redirects: ChartRedirect[] = []
     @observable pageviews?: RawPageview = undefined
     @observable allTopics: Topic[] = []
 
-    static contextType = AdminAppContext
-
     patchConfig: GrapherInterface = {}
     parentConfig: GrapherInterface | undefined = undefined
 
@@ -143,9 +144,8 @@ export class ChartEditorPage
         this.refresh()
     }
 
-    // TODO(inheritance)
     // This funny construction allows the "new chart" link to work by forcing an update
-    // even if the props don't change
+    // even if the props don't change. This fix used to work, but doesn't anymore.
     // UNSAFE_componentWillReceiveProps(): void {
     //     setTimeout(() => this.refresh(), 0)
     // }

adminSiteClient/ChartEditorView.tsx

@@ -348,13 +348,6 @@ export class ChartEditorView<
         )
     }
 
-    // TODO(inheritance)
-    // This funny construction allows the "new chart" link to work by forcing an update
-    // even if the props don't change
-    // UNSAFE_componentWillReceiveProps(): void {
-    //     setTimeout(() => this.refresh(), 0)
-    // }
-
     disposers: IReactionDisposer[] = []
     componentWillUnmount(): void {
         this.disposers.forEach((dispose) => dispose())

adminSiteClient/EditorCustomizeTab.tsx

@@ -459,7 +459,6 @@ export class EditorCustomizeTab<
 > extends React.Component<{
     editor: Editor
 }> {
-    // TODO
     static contextType = ChartEditorContext
 
     @computed get errorMessages() {

adminSiteClient/EditorDataTab.tsx

@@ -84,21 +84,15 @@ class EntityItem extends React.Component<EntityItemProps> {
 }
 
 @observer
-export class KeysSection extends React.Component<{
-    editor: AbstractChartEditor
-}> {
+export class KeysSection extends React.Component<{ grapher: Grapher }> {
     @observable.ref dragKey?: EntityName
 
-    @computed get grapher() {
-        return this.props.editor.grapher
-    }
-
     @action.bound onAddKey(entityName: EntityName) {
-        this.grapher.selection.selectEntity(entityName)
+        this.props.grapher.selection.selectEntity(entityName)
     }
 
     @action.bound onDragEnd(result: DropResult) {
-        const { selection } = this.grapher
+        const { selection } = this.props.grapher
         const { source, destination } = result
         if (!destination) return
 
@@ -111,15 +105,12 @@ export class KeysSection extends React.Component<{
     }
 
     render() {
-        const { grapher } = this
+        const { grapher } = this.props
         const { selection } = grapher
         const { unselectedEntityNames, selectedEntityNames } = selection
 
         return (
             <Section name="Data to show">
-                {this.props.editor.isPropertyInherited("selectedEntityNames")
-                    ? "inherited"
-                    : "not inherited"}
                 <SelectField
                     onValue={this.onAddKey}
                     value="Select data"
@@ -287,7 +278,7 @@ export class EditorDataTab<
                         </label>
                     </div>
                 </Section>
-                <KeysSection editor={editor} />
+                <KeysSection grapher={editor.grapher} />
                 {features.canSpecifyMissingDataStrategy && (
                     <MissingDataSection editor={this.props.editor} />
                 )}

adminSiteClient/EditorHistoryTab.tsx

@@ -127,7 +127,8 @@ export class EditorHistoryTab extends React.Component<{ editor: ChartEditor }> {
     }
 
     @action.bound copyYamlToClipboard() {
-        // Use the Clipboard API to copy the config into the users clipboard
+        // Avoid modifying the original JSON object
+        // Due to mobx memoizing computed values, the JSON can be mutated.
         const patchConfig = {
             ...this.props.editor.patchConfig,
         }
@@ -136,6 +137,7 @@ export class EditorHistoryTab extends React.Component<{ editor: ChartEditor }> {
         delete patchConfig.version
         delete patchConfig.isPublished
         const chartConfigAsYaml = YAML.stringify(patchConfig)
+        // Use the Clipboard API to copy the config into the users clipboard
         void copyToClipboard(chartConfigAsYaml)
         notification["success"]({
             message: "Copied YAML to clipboard",

adminSiteClient/IndicatorChartEditor.ts

@@ -27,8 +27,7 @@ export class IndicatorChartEditor extends AbstractChartEditor<IndicatorChartEdit
     async saveGrapher({
         onError,
     }: { onError?: () => void } = {}): Promise<void> {
-        // TODO(inheritance)
-        console.log("save indicator chart")
+        console.log("Implementation missing")
         onError?.()
     }
 }

adminSiteClient/IndicatorChartEditorPage.tsx

@@ -1,9 +1,8 @@
 import React from "react"
 import { observer } from "mobx-react"
-import { observable, computed, action } from "mobx"
+import { computed, action } from "mobx"
 import { isEmpty } from "@ourworldindata/utils"
 import { GrapherInterface, DimensionProperty } from "@ourworldindata/types"
-import { Grapher } from "@ourworldindata/grapher"
 import { Admin } from "./Admin.js"
 import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js"
 import { ChartEditorView, ChartEditorViewManager } from "./ChartEditorView.js"
@@ -21,8 +20,6 @@ export class IndicatorChartEditorPage
         IndicatorChartEditorManager,
         ChartEditorViewManager<IndicatorChartEditor>
 {
-    @observable.ref grapher = new Grapher()
-
     static contextType = AdminAppContext
     context!: AdminAppContextType
 
@@ -59,9 +56,8 @@ export class IndicatorChartEditorPage
         this.refresh()
     }
 
-    // TODO(inheritance)
     // This funny construction allows the "new chart" link to work by forcing an update
-    // even if the props don't change
+    // even if the props don't change. This fix used to work, but doesn't anymore.
     // UNSAFE_componentWillReceiveProps(): void {
     //     setTimeout(() => this.refresh(), 0)
     // }

adminSiteClient/SaveButtons.tsx

@@ -10,15 +10,12 @@ import { IndicatorChartEditor } from "./IndicatorChartEditor.js"
 export class SaveButtonsForChart extends React.Component<{
     editor: ChartEditor
 }> {
-    // TODO
     static contextType = ChartEditorContext
 
     @action.bound onSaveChart() {
         void this.props.editor.saveGrapher()
     }
 
-    // TODO
-
     @action.bound onSaveAsNew() {
         void this.props.editor.saveAsNewGrapher()
     }
@@ -66,7 +63,7 @@ export class SaveButtonsForChart extends React.Component<{
                     disabled={isSavingDisabled}
                 >
                     Save as new
-                </button>
+                </button>{" "}
                 <button
                     className="btn btn-danger"
                     onClick={this.onPublishToggle}

adminSiteClient/VariableEditPage.tsx

@@ -55,7 +55,7 @@ interface VariablePageData
     extends Omit<OwidVariableWithDataAndSource, "source"> {
     datasetNamespace: string
     charts: ChartListItem[]
-    grapherConfig: GrapherInterface | undefined // admin+ETL merged
+    grapherConfig: GrapherInterface | undefined
     grapherConfigETL: GrapherInterface | undefined
     source: { id: number; name: string }
     origins: OwidOrigin[]
@@ -124,7 +124,6 @@ class VariableEditable
 @observer
 class VariableEditor extends React.Component<{
     variable: VariablePageData
-    grapherConfigETL?: GrapherInterface
 }> {
     @observable newVariable!: VariableEditable
     @observable isDeleted: boolean = false
@@ -150,12 +149,8 @@ class VariableEditor extends React.Component<{
         )
     }
 
-    @computed get grapherConfigETL(): GrapherInterface | undefined {
-        return this.props.grapherConfigETL
-    }
-
     render() {
-        const { variable, grapherConfigETL } = this.props
+        const { variable } = this.props
         const { newVariable, isV2MetadataVariable } = this
 
         const pathFragments = variable.catalogPath
@@ -413,7 +408,9 @@ class VariableEditor extends React.Component<{
                                         label="Grapher Config ETL"
                                         field="v"
                                         store={{
-                                            v: YAML.stringify(grapherConfigETL),
+                                            v: YAML.stringify(
+                                                variable.grapherConfigETL
+                                            ),
                                         }}
                                         disabled
                                         textarea

adminSiteClient/VariableSelector.tsx

@@ -501,7 +501,6 @@ export class VariableSelector<
     dispose!: IReactionDisposer
     base: React.RefObject<HTMLDivElement> = React.createRef()
     componentDidMount() {
-        // void this.props.editor.loadVariableUsageCounts() // TODO???
         this.initChosenVariablesAndNamespaces()
     }