Skip to content

Commit

Permalink
✨ Extracts most of Grapher's props into a new GrapherState class
Browse files Browse the repository at this point in the history
  • Loading branch information
danyx23 committed Feb 21, 2025
1 parent ed35696 commit 52e6212
Show file tree
Hide file tree
Showing 67 changed files with 4,405 additions and 3,761 deletions.
10 changes: 2 additions & 8 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@
"skipFiles": [
"<node_internals>/**"
],
"skipFiles": [
"<node_internals>/**"
],
"type": "node"
},
{
Expand All @@ -28,11 +25,8 @@
"${fileBasenameNoExtension}.js",
"--watch"
],
"args": [
"${fileBasenameNoExtension}.js",
"--watch"
],
"console": "integratedTerminal"
"console": "integratedTerminal",
"runtimeExecutable": "/run/user/1000/fnm_multishells/90830_1737588026933/bin/node"
// "internalConsoleOptions": "neverOpen"
},
{
Expand Down
3 changes: 2 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@
},
"Prettier-SQL.keywordCase": "upper",
"Prettier-SQL.SQLFlavourOverride": "mysql",
"Prettier-SQL.expressionWidth": 80
"Prettier-SQL.expressionWidth": 80,
"prettier.semi": false
}
34 changes: 20 additions & 14 deletions adminSiteClient/AbstractChartEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,14 @@ import {
import { action, computed, observable, when } from "mobx"
import { EditorFeatures } from "./EditorFeatures.js"
import { Admin } from "./Admin.js"
import { defaultGrapherConfig, Grapher } from "@ourworldindata/grapher"
import {
defaultGrapherConfig,
getCachingInputTableFetcher,
GrapherState,
} from "@ourworldindata/grapher"
import { ChartViewMinimalInformation } from "./ChartEditor.js"
import { IndicatorChartInfo } from "./IndicatorChartEditor.js"
import { DATA_API_URL } from "../settings/clientSettings.js"

export type EditorTab =
| "basic"
Expand Down Expand Up @@ -48,7 +53,8 @@ export abstract class AbstractChartEditor<
> {
manager: Manager

@observable.ref grapher = new Grapher()
@observable.ref grapherState = new GrapherState({})
cachingGrapherDataLoader = getCachingInputTableFetcher(DATA_API_URL)

Check failure on line 57 in adminSiteClient/AbstractChartEditor.ts

View workflow job for this annotation

GitHub Actions / bundlewatch

Expected 2 arguments, but got 1.

Check failure on line 57 in adminSiteClient/AbstractChartEditor.ts

View workflow job for this annotation

GitHub Actions / testcheck

Expected 2 arguments, but got 1.

Check failure on line 57 in adminSiteClient/AbstractChartEditor.ts

View workflow job for this annotation

GitHub Actions / testdbcheck

Expected 2 arguments, but got 1.

Check failure on line 57 in adminSiteClient/AbstractChartEditor.ts

View workflow job for this annotation

GitHub Actions / bundlewatch

Expected 2 arguments, but got 1.

Check failure on line 57 in adminSiteClient/AbstractChartEditor.ts

View workflow job for this annotation

GitHub Actions / testcheck

Expected 2 arguments, but got 1.

Check failure on line 57 in adminSiteClient/AbstractChartEditor.ts

View workflow job for this annotation

GitHub Actions / testdbcheck

Expected 2 arguments, but got 1.
@observable.ref currentRequest: Promise<any> | undefined // Whether the current chart state is saved or not
@observable.ref tab: EditorTab = "basic"
@observable.ref errorMessage?: { title: string; content: string }
Expand All @@ -58,7 +64,7 @@ export abstract class AbstractChartEditor<

// 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
// if inheritance is enabled, the parent config is applied to grapherState
@observable.ref isInheritanceEnabled: boolean | undefined = undefined

constructor(props: { manager: Manager }) {
Expand All @@ -80,14 +86,14 @@ export abstract class AbstractChartEditor<
)

when(
() => this.grapher.hasData && this.grapher.isReady,
() => this.grapherState.hasData && this.grapherState.isReady,
() => (this.savedPatchConfig = this.patchConfig)
)
}

abstract get references(): References | undefined

/** original grapher config used to init the grapher instance */
/** original grapher config used to init the grapherState instance */
@computed get originalGrapherConfig(): GrapherInterface {
const { patchConfig, parentConfig, isInheritanceEnabled } = this.manager
if (!isInheritanceEnabled) return patchConfig
Expand All @@ -96,7 +102,7 @@ export abstract class AbstractChartEditor<

/** live-updating config */
@computed get liveConfig(): GrapherInterface {
return this.grapher.object
return this.grapherState.object
}

@computed get liveConfigWithDefaults(): GrapherInterface {
Expand Down Expand Up @@ -144,9 +150,9 @@ export abstract class AbstractChartEditor<
}

@action.bound updateLiveGrapher(config: GrapherInterface): void {
this.grapher.reset()
this.grapher.updateFromObject(config)
this.grapher.updateAuthoredVersion(config)
this.grapherState.reset()
this.grapherState.updateFromObject(config)
this.grapherState.updateAuthoredVersion(config)
}

// only works for top-level properties
Expand All @@ -166,21 +172,21 @@ export abstract class AbstractChartEditor<
}

@computed get invalidFocusedSeriesNames(): SeriesName[] {
const { grapher } = this
const { grapherState } = this

// if focusing is not supported, then all focused series are invalid
if (!this.features.canHighlightSeries) {
return grapher.focusArray.seriesNames
return grapherState.focusArray.seriesNames
}

// find invalid focused series
const availableSeriesNames = grapher.chartSeriesNames
const focusedSeriesNames = grapher.focusArray.seriesNames
const availableSeriesNames = grapherState.chartSeriesNames
const focusedSeriesNames = grapherState.focusArray.seriesNames
return difference(focusedSeriesNames, availableSeriesNames)
}

@action.bound removeInvalidFocusedSeriesNames(): void {
this.grapher.focusArray.remove(...this.invalidFocusedSeriesNames)
this.grapherState.focusArray.remove(...this.invalidFocusedSeriesNames)
}

abstract get isNewGrapher(): boolean
Expand Down
36 changes: 18 additions & 18 deletions adminSiteClient/ChartEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,9 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {

@computed get availableTabs(): EditorTab[] {
const tabs: EditorTab[] = ["basic", "data", "text", "customize"]
if (this.grapher.hasMapTab) tabs.push("map")
if (this.grapher.isScatter) tabs.push("scatter")
if (this.grapher.isMarimekko) tabs.push("marimekko")
if (this.grapherState.hasMapTab) tabs.push("map")
if (this.grapherState.isScatter) tabs.push("scatter")
if (this.grapherState.isMarimekko) tabs.push("marimekko")
tabs.push("revisions")
tabs.push("refs")
tabs.push("export")
Expand All @@ -100,14 +100,14 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
}

@computed get isNewGrapher() {
return this.grapher.id === undefined
return this.grapherState.id === undefined
}

@action.bound async updateParentConfig() {
const currentParentIndicatorId =
this.parentConfig?.dimensions?.[0].variableId
const newParentIndicatorId = getParentVariableIdFromChartConfig(
this.grapher.object
this.grapherState.object
)

// no-op if the parent indicator hasn't changed
Expand Down Expand Up @@ -138,12 +138,12 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
async saveGrapher({
onError,
}: { onError?: () => void } = {}): Promise<void> {
const { grapher, isNewGrapher, patchConfig } = this
const { grapherState, isNewGrapher, patchConfig } = this

// Chart title and slug may be autocalculated from data, in which case they won't be in props
// But the server will need to know what we calculated in order to do its job
if (!patchConfig.title) patchConfig.title = grapher.displayTitle
if (!patchConfig.slug) patchConfig.slug = grapher.displaySlug
if (!patchConfig.title) patchConfig.title = grapherState.displayTitle
if (!patchConfig.slug) patchConfig.slug = grapherState.displaySlug

// it only makes sense to enable inheritance if the chart has a parent
const shouldEnableInheritance =
Expand All @@ -154,7 +154,7 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
})
const targetUrl = isNewGrapher
? `/api/charts?${query}`
: `/api/charts/${grapher.id}?${query}`
: `/api/charts/${grapherState.id}?${query}`

const json = await this.manager.admin.requestJSON(
targetUrl,
Expand All @@ -165,12 +165,12 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
if (json.success) {
if (isNewGrapher) {
this.newChartId = json.chartId
this.grapher.id = json.chartId
this.grapherState.id = json.chartId
this.savedPatchConfig = json.savedPatch
this.isInheritanceEnabled = shouldEnableInheritance
} else {
runInAction(() => {
grapher.version += 1
grapherState.version += 1
this.logs.unshift(json.newLog)
this.savedPatchConfig = json.savedPatch
this.isInheritanceEnabled = shouldEnableInheritance
Expand Down Expand Up @@ -213,13 +213,13 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
async saveAsChartView(
name: string
): Promise<{ success: boolean; errorMsg?: string }> {
const { patchConfig, grapher } = this
const { patchConfig, grapherState } = this

const chartJson = omit(patchConfig, CHART_VIEW_PROPS_TO_OMIT)

const body = {
name,
parentChartId: grapher.id,
parentChartId: grapherState.id,
config: chartJson,
}

Expand All @@ -239,12 +239,12 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
}

publishGrapher(): void {
const url = `${BAKED_GRAPHER_URL}/${this.grapher.displaySlug}`
const url = `${BAKED_GRAPHER_URL}/${this.grapherState.displaySlug}`

if (window.confirm(`Publish chart at ${url}?`)) {
this.grapher.isPublished = true
this.grapherState.isPublished = true
void this.saveGrapher({
onError: () => (this.grapher.isPublished = undefined),
onError: () => (this.grapherState.isPublished = undefined),
})
}
}
Expand All @@ -255,9 +255,9 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
? "WARNING: This chart might be referenced from public posts, please double check before unpublishing. Try to remove the chart anyway?"
: "Are you sure you want to unpublish this chart?"
if (window.confirm(message)) {
this.grapher.isPublished = undefined
this.grapherState.isPublished = undefined
void this.saveGrapher({
onError: () => (this.grapher.isPublished = true),
onError: () => (this.grapherState.isPublished = true),
})
}
}
Expand Down
43 changes: 20 additions & 23 deletions adminSiteClient/ChartEditorView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
GrapherStaticFormat,
DimensionProperty,
} from "@ourworldindata/types"
import { Grapher } from "@ourworldindata/grapher"
import { Grapher, GrapherState } from "@ourworldindata/grapher"
import { Admin } from "./Admin.js"
import { getFullReferencesCount, isChartEditorInstance } from "./ChartEditor.js"
import { EditorBasicTab } from "./EditorBasicTab.js"
Expand Down Expand Up @@ -109,7 +109,9 @@ export class ChartEditorView<
}> {
@observable.ref database = new EditorDatabase({})
@observable details: DetailDictionary = {}
@observable.ref grapherElement?: React.ReactElement
@computed get grapherState(): GrapherState {
return this.manager.editor.grapherState
}

@observable simulateVisionDeficiency?: VisionDeficiency

Expand All @@ -125,20 +127,16 @@ export class ChartEditorView<

@action.bound private updateGrapher(): void {
const config = this.manager.editor.originalGrapherConfig
const grapherConfig = {
// 2025-01-04 Daniel Not sure this is the best way to do things
this.grapherState.updateFromObject({
...config,
// binds the grapher instance to this.grapher
getGrapherInstance: (grapher: Grapher) => {
this.manager.editor.grapher = grapher
},
dataApiUrlForAdmin:
this.manager.admin.settings.DATA_API_FOR_ADMIN_UI, // passed this way because clientSettings are baked and need a recompile to be updated
bounds: this.bounds,
staticFormat: this.staticFormat,
}
this.manager.editor.grapher.renderToStatic =
})
this.manager.editor.grapherState.renderToStatic =
!!this.editor?.showStaticPreview
this.grapherElement = <Grapher {...grapherConfig} />
this._isGrapherSet = true
}

Expand Down Expand Up @@ -196,7 +194,7 @@ export class ChartEditorView<
@computed private get bounds(): Bounds {
return this.isMobilePreview
? new Bounds(0, 0, 380, 525)
: this.manager.editor.grapher.defaultBounds
: this.grapherState.defaultBounds
}

@computed private get staticFormat(): GrapherStaticFormat {
Expand All @@ -209,15 +207,15 @@ export class ChartEditorView<
// these may point to non-existent details e.g. ["not_a_real_term", "pvotery"]
@computed
get currentDetailReferences(): DetailReferences {
const { grapher } = this.manager.editor
const { grapherState } = this.manager.editor
return {
subtitle: extractDetailsFromSyntax(grapher.currentSubtitle),
note: extractDetailsFromSyntax(grapher.note ?? ""),
subtitle: extractDetailsFromSyntax(grapherState.currentSubtitle),
note: extractDetailsFromSyntax(grapherState.note ?? ""),
axisLabelX: extractDetailsFromSyntax(
grapher.xAxisConfig.label ?? ""
grapherState.xAxisConfig.label ?? ""
),
axisLabelY: extractDetailsFromSyntax(
grapher.yAxisConfig.label ?? ""
grapherState.yAxisConfig.label ?? ""
),
}
}
Expand Down Expand Up @@ -286,7 +284,7 @@ export class ChartEditorView<
[DimensionProperty.table]: [], // not used
}

this.manager.editor.grapher.dimensionSlots.forEach((slot) => {
this.grapherState.dimensionSlots.forEach((slot) => {
slot.dimensions.forEach((dimension, dimensionIndex) => {
const details = extractDetailsFromSyntax(
dimension.display.name ?? ""
Expand Down Expand Up @@ -353,7 +351,7 @@ export class ChartEditorView<
}

renderReady(editor: Editor): React.ReactElement {
const { grapher, availableTabs } = editor
const { grapherState, availableTabs } = editor

const chartEditor = isChartEditorInstance(editor) ? editor : undefined

Expand Down Expand Up @@ -424,10 +422,10 @@ export class ChartEditorView<
/>
)}
{editor.tab === "scatter" && (
<EditorScatterTab grapher={grapher} />
<EditorScatterTab grapherState={grapherState} />
)}
{editor.tab === "marimekko" && (
<EditorMarimekkoTab grapher={grapher} />
<EditorMarimekkoTab grapherState={grapherState} />
)}
{editor.tab === "map" && (
<EditorMapTab editor={editor} />
Expand Down Expand Up @@ -457,10 +455,9 @@ export class ChartEditorView<
</div>
<div className="chart-editor-view">
<figure
data-grapher-src
style={{
minHeight: editor.showStaticPreview
? grapher.staticBoundsWithDetails.height
? grapherState.staticBoundsWithDetails.height
: undefined,
boxShadow: editor.showStaticPreview
? "0px 4px 40px rgba(0, 0, 0, 0.2)"
Expand All @@ -470,7 +467,7 @@ export class ChartEditorView<
`url(#${this.simulateVisionDeficiency.id})`,
}}
>
{this.grapherElement}
<Grapher grapherState={this.grapherState} />
</figure>
<div>
<div
Expand Down
6 changes: 3 additions & 3 deletions adminSiteClient/ChartViewEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ export class ChartViewEditor extends AbstractChartEditor<ChartViewEditorManager>
@computed
get availableTabs(): EditorTab[] {
const tabs: EditorTab[] = ["basic", "data", "text", "customize"]
if (this.grapher.hasMapTab) tabs.push("map")
if (this.grapher.isScatter) tabs.push("scatter")
if (this.grapher.isMarimekko) tabs.push("marimekko")
if (this.grapherState.hasMapTab) tabs.push("map")
if (this.grapherState.isScatter) tabs.push("scatter")
if (this.grapherState.isMarimekko) tabs.push("marimekko")
tabs.push("refs")
tabs.push("export")
tabs.push("debug")
Expand Down
Loading

0 comments on commit 52e6212

Please sign in to comment.