diff --git a/adminSiteClient/AbstractChartEditor.ts b/adminSiteClient/AbstractChartEditor.ts index c425e58797f..427d44ff592 100644 --- a/adminSiteClient/AbstractChartEditor.ts +++ b/adminSiteClient/AbstractChartEditor.ts @@ -42,6 +42,12 @@ export abstract class AbstractChartEditor< @observable.ref savedPatchConfig: GrapherInterface = {} @observable.ref parentConfig: GrapherInterface | undefined = undefined + // TODO: does this work? + completeDefaultConfig = mergeGrapherConfigs( + new Grapher().fullObject, + defaultGrapherConfig + ) + constructor(props: { manager: Manager }) { this.manager = props.manager this.previewMode = @@ -67,22 +73,25 @@ export abstract class AbstractChartEditor< } @computed get liveConfig(): GrapherInterface { - return this.grapher.object + return this.grapher.fullObject + } + + @computed get fullConfig(): GrapherInterface { + // todo: remove serialised keys that are not part of the schema + return this.liveConfig } @computed get patchConfig(): GrapherInterface { - const { liveConfig, parentConfig } = this - if (!parentConfig) return liveConfig - // Grapher's toObject method doesn't include default values, - // but the patch config might need to overwrite non-default values - // from the parent layer. That's why we merge the default grapher config - // in here. The parent config also contains defaults, meaning we're - // getting rid of the defaults when we diff against the parent config below. - const liveConfigWithDefaults = mergeGrapherConfigs( - defaultGrapherConfig, - liveConfig - ) - return diffGrapherConfigs(liveConfigWithDefaults, parentConfig) + const { fullConfig, parentConfig } = this + if (!parentConfig) { + return diffGrapherConfigs(fullConfig, this.completeDefaultConfig) + } + return diffGrapherConfigs(fullConfig, parentConfig) + } + + @computed get parentConfigWithoutDefaults(): GrapherInterface | undefined { + if (!this.parentConfig) return undefined + return diffGrapherConfigs(this.parentConfig, defaultGrapherConfig) } @computed get isModified(): boolean { @@ -96,6 +105,15 @@ export abstract class AbstractChartEditor< return new EditorFeatures(this) } + // TODO: only works for first level at the moment + isPropertyInherited(property: keyof GrapherInterface): boolean { + if (!this.parentConfigWithoutDefaults) return false + return ( + !Object.hasOwn(this.patchConfig, property) && + Object.hasOwn(this.parentConfigWithoutDefaults, property) + ) + } + abstract get isNewGrapher(): boolean abstract get availableTabs(): EditorTab[] diff --git a/adminSiteClient/ChartEditor.ts b/adminSiteClient/ChartEditor.ts index d728f5d284b..aef4361cf41 100644 --- a/adminSiteClient/ChartEditor.ts +++ b/adminSiteClient/ChartEditor.ts @@ -18,7 +18,7 @@ import { } from "@ourworldindata/utils" import { action, computed, observable, runInAction } from "mobx" import { BAKED_GRAPHER_URL } from "../settings/clientSettings.js" -import { defaultGrapherConfig } from "@ourworldindata/grapher" +import { defaultGrapherConfig, Grapher } from "@ourworldindata/grapher" import { AbstractChartEditor, AbstractChartEditorManager, @@ -96,12 +96,18 @@ export class ChartEditor extends AbstractChartEditor { } @action.bound async updateParentConfig() { + const currentIndicatorId = this.parentConfig?.dimensions?.[0].variableId const newIndicatorId = getParentIndicatorIdFromChartConfig( - this.liveConfig + this.fullConfig ) - const currentIndicatorId = this.parentConfig?.dimensions?.[0].variableId - if (newIndicatorId !== currentIndicatorId) { - this.parentConfig = newIndicatorId + + // fetch the new parent config if the indicator has changed + let newParentConfig: GrapherInterface | undefined + if ( + currentIndicatorId === undefined || + newIndicatorId !== currentIndicatorId + ) { + newParentConfig = newIndicatorId ? await fetchParentConfigForChart( this.manager.admin, newIndicatorId @@ -109,18 +115,32 @@ export class ChartEditor extends AbstractChartEditor { : undefined } - // it's intentional that we don't use this.patchConfig here - const patchConfig = diffGrapherConfigs( - this.liveConfig, - this.parentConfig ?? {} + const currentPatchConfig = this.patchConfig + + const completeConfig = mergeGrapherConfigs( + new Grapher().fullObject, + defaultGrapherConfig + ) + + // first, remove outdated parent values from the current grapher instance + this.grapher.updateFromObject( + mergeGrapherConfigs(completeConfig, currentPatchConfig) ) - const config = mergeGrapherConfigs(this.parentConfig ?? {}, patchConfig) - this.grapher.updateFromObject(config) + // then, merge the new parent values into the current grapher instance + if (newParentConfig) { + this.grapher.updateFromObject( + mergeGrapherConfigs(newParentConfig, currentPatchConfig) + ) + } + + // this.grapher.updateFromObject(updatedObject) // TODO(inheritance): what does this do? is this necessary? - this.grapher.updateAuthoredVersion({ - ...this.grapher.toObject(), - }) + this.grapher.updateAuthoredVersion( + mergeGrapherConfigs(newParentConfig ?? {}, currentPatchConfig) + ) + + this.parentConfig = newParentConfig } async saveGrapher({ @@ -211,7 +231,10 @@ export async function fetchParentConfigForChart( const indicatorChart = await admin.getJSON( `/api/variables/mergedGrapherConfig/${indicatorId}.json` ) - return mergeGrapherConfigs(defaultGrapherConfig, indicatorChart) + return mergeGrapherConfigs( + mergeGrapherConfigs(new Grapher().fullObject, defaultGrapherConfig), + indicatorChart + ) } export function isChartEditorInstance( diff --git a/adminSiteClient/EditorBasicTab.tsx b/adminSiteClient/EditorBasicTab.tsx index 639609a9bab..388dfed5938 100644 --- a/adminSiteClient/EditorBasicTab.tsx +++ b/adminSiteClient/EditorBasicTab.tsx @@ -82,7 +82,12 @@ class DimensionSlotView< this.isSelectingVariables = false - this.updateDimensionsAndRebuildTable(dimensionConfigs) + // TODO: why does this sometimes fail? + try { + this.updateDimensionsAndRebuildTable(dimensionConfigs) + } catch { + console.log("updateDimensionsAndRebuildTable failed") + } this.updateParentConfig() } @@ -138,6 +143,7 @@ class DimensionSlotView< } componentDidMount() { + // TODO: re-enable // We want to add the reaction only after the grapher is loaded, so we don't update the initial chart (as configured) by accident. when( () => this.grapher.isReady, diff --git a/adminSiteClient/EditorDataTab.tsx b/adminSiteClient/EditorDataTab.tsx index af28179a3b3..e32c3e2abb3 100644 --- a/adminSiteClient/EditorDataTab.tsx +++ b/adminSiteClient/EditorDataTab.tsx @@ -84,15 +84,21 @@ class EntityItem extends React.Component { } @observer -export class KeysSection extends React.Component<{ grapher: Grapher }> { +export class KeysSection extends React.Component<{ + editor: AbstractChartEditor +}> { @observable.ref dragKey?: EntityName + @computed get grapher() { + return this.props.editor.grapher + } + @action.bound onAddKey(entityName: EntityName) { - this.props.grapher.selection.selectEntity(entityName) + this.grapher.selection.selectEntity(entityName) } @action.bound onDragEnd(result: DropResult) { - const { selection } = this.props.grapher + const { selection } = this.grapher const { source, destination } = result if (!destination) return @@ -105,12 +111,16 @@ export class KeysSection extends React.Component<{ grapher: Grapher }> { } render() { - const { grapher } = this.props + const { grapher } = this const { selection } = grapher const { unselectedEntityNames, selectedEntityNames } = selection return (
+ from parent config:{" "} + {this.props.editor + .isPropertyInherited("selectedEntityNames") + .toString()} { key={entityName} grapher={grapher} entityName={entityName} - onRemove={() => + onRemove={() => { selection.deselectEntity( entityName ) - } + console.log( + selection.numSelectedEntities + ) + }} /> )} @@ -278,7 +291,7 @@ export class EditorDataTab<
- + {features.canSpecifyMissingDataStrategy && ( )} diff --git a/adminSiteClient/EditorHistoryTab.tsx b/adminSiteClient/EditorHistoryTab.tsx index 4c0e8baf5f8..22f2aa7f8d7 100644 --- a/adminSiteClient/EditorHistoryTab.tsx +++ b/adminSiteClient/EditorHistoryTab.tsx @@ -3,10 +3,15 @@ import { observer } from "mobx-react" import { ChartEditor, Log } from "./ChartEditor.js" import { Section, Timeago } from "./Forms.js" import { computed, action, observable } from "mobx" -import { Json, copyToClipboard } from "@ourworldindata/utils" +import { + Json, + copyToClipboard, + diffGrapherConfigs, +} from "@ourworldindata/utils" import YAML from "yaml" import { notification, Modal } from "antd" import ReactDiffViewer, { DiffMethod } from "react-diff-viewer-continued" +import { defaultGrapherConfig } from "@ourworldindata/grapher" function LogCompareModal({ log, @@ -128,14 +133,14 @@ export class EditorHistoryTab extends React.Component<{ editor: ChartEditor }> { @action.bound copyYamlToClipboard() { // Use the Clipboard API to copy the config into the users clipboard - const chartConfigObject = { + const patchConfig = { ...this.props.editor.patchConfig, } - delete chartConfigObject.id - delete chartConfigObject.dimensions - delete chartConfigObject.version - delete chartConfigObject.isPublished - const chartConfigAsYaml = YAML.stringify(chartConfigObject) + delete patchConfig.id + delete patchConfig.dimensions + delete patchConfig.version + delete patchConfig.isPublished + const chartConfigAsYaml = YAML.stringify(patchConfig) void copyToClipboard(chartConfigAsYaml) notification["success"]({ message: "Copied YAML to clipboard", @@ -146,11 +151,8 @@ export class EditorHistoryTab extends React.Component<{ editor: ChartEditor }> { } render() { - // Avoid modifying the original JSON object - // Due to mobx memoizing computed values, the JSON can be mutated. - const chartConfigObject = { - ...this.props.editor.patchConfig, - } + const { patchConfig, fullConfig, parentConfigWithoutDefaults } = + this.props.editor return (
{this.logs.map((log, i) => ( @@ -178,9 +180,31 @@ export class EditorHistoryTab extends React.Component<{ editor: ChartEditor }> { rows={7} readOnly className="form-control" - value={JSON.stringify(chartConfigObject, undefined, 2)} + value={JSON.stringify(patchConfig, undefined, 2)} /> +
+