From 2d53d5a760527a9d2114db685c44ce7fa5b1c415 Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Sun, 2 Feb 2025 17:36:43 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Move=20remaining=20computed=20props?= =?UTF-8?q?=20into=20grapherstate.=20This=20fixes=20a=20few=20bugs.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The main channel through which this led to undesired behavior is that we used to pass the full Grapher as manager into components like the Footer; we now only pass the GrapherState. But because the interfaces that outline managers (like that of the footer) often use optional properties, the type system didn't flag that some of the computed properties were missing now on GrapherState. This commit moves the remaining computed state props onto GrapherState. --- .../grapher/src/core/Grapher.tsx | 138 ++++++++---------- 1 file changed, 63 insertions(+), 75 deletions(-) diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx index 47dbbff075..33e6275cf7 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -3121,32 +3121,10 @@ export class GrapherState { } @observable isShareMenuActive = false -} - -export interface GrapherProps { - grapherState: GrapherState -} - -@observer -export class Grapher extends React.Component { - @computed get grapherState(): GrapherState { - return this.props.grapherState - } - - // #region Observable props not in any interface - - analytics = new GrapherAnalytics( - this.props.grapherState.initialOptions.env ?? "" - ) - - // stored on Grapher so state is preserved when switching to full-screen mode - @observable - private legacyVariableDataJson?: MultipleOwidVariableDataDimensionsMap - private hasLoggedGAViewEvent = false - @observable private hasBeenVisible = false - @observable private uncaughtError?: Error - @observable slideShow?: SlideShowController + // Computed props that are not required by any inteface but can be optional in interfaces + // and could then lead to changed behavior vs previously if now only GrapherState is passed into + // a component. /** * Whether the chart is rendered in an Admin context (e.g. on owid.cloud). @@ -3162,9 +3140,9 @@ export class Grapher extends React.Component { @computed get shouldLinkToOwid(): boolean { if ( - this.grapherState.isEmbeddedInAnOwidPage || - this.grapherState.isExportingToSvgOrPng || - !this.grapherState.isInIFrame + this.isEmbeddedInAnOwidPage || + this.isExportingToSvgOrPng || + !this.isInIFrame ) return false @@ -3172,39 +3150,31 @@ export class Grapher extends React.Component { } @computed.struct private get variableIds(): number[] { - return uniq(this.grapherState.dimensions.map((d) => d.variableId)) + return uniq(this.dimensions.map((d) => d.variableId)) } @computed get hasOWIDLogo(): boolean { return ( - !this.grapherState.hideLogo && - (this.grapherState.logo === undefined || - this.grapherState.logo === "owid") + !this.hideLogo && (this.logo === undefined || this.logo === "owid") ) } @computed get xScaleType(): ScaleType | undefined { - return this.grapherState.xAxis.scaleType + return this.xAxis.scaleType } @computed get hasYDimension(): boolean { - return this.grapherState.dimensions.some( - (d) => d.property === DimensionProperty.y - ) - } - - @computed get defaultBounds(): Bounds { - return new Bounds(0, 0, DEFAULT_GRAPHER_WIDTH, DEFAULT_GRAPHER_HEIGHT) + return this.dimensions.some((d) => d.property === DimensionProperty.y) } @computed get cacheTag(): string { - return this.grapherState.version.toString() + return this.version.toString() } // Filter data to what can be display on the map (across all times) @computed get mappableData(): OwidVariableRow[] { - return this.grapherState.inputTable - .get(this.grapherState.mapColumnSlug) + return this.inputTable + .get(this.mapColumnSlug) .owidRows.filter((row) => isOnTheMap(row.entityName)) } @@ -3213,44 +3183,40 @@ export class Grapher extends React.Component { } @computed get hideFullScreenButton(): boolean { - if (this.grapherState.isInFullScreenMode) return false + if (this.isInFullScreenMode) return false // hide the full screen button if the full screen height // is barely larger than the current chart height - const fullScreenHeight = this.grapherState.windowInnerHeight! - return fullScreenHeight < this.grapherState.frameBounds.height + 80 + const fullScreenHeight = this.windowInnerHeight! + return fullScreenHeight < this.frameBounds.height + 80 } @computed get sidePanelBounds(): Bounds | undefined { - if (!this.grapherState.isEntitySelectorPanelActive) return + if (!this.isEntitySelectorPanelActive) return return new Bounds( 0, // not in use; intentionally set to zero 0, // not in use; intentionally set to zero - this.grapherState.frameBounds.width - - this.grapherState.captionedChartBounds.width, - this.grapherState.captionedChartBounds.height + this.frameBounds.width - this.captionedChartBounds.width, + this.captionedChartBounds.height ) } @computed get containerElement(): HTMLDivElement | undefined { - return this.grapherState.base.current || undefined + return this.base.current || undefined } // the header and footer don't rely on the base font size unless explicitly specified @computed get useBaseFontSize(): boolean { - return ( - this.props.grapherState.initialOptions.baseFontSize !== undefined || - this.grapherState.isStatic - ) + return this.initialOptions.baseFontSize !== undefined || this.isStatic } @computed get hasRelatedQuestion(): boolean { if ( - this.grapherState.hideRelatedQuestion || - !this.grapherState.relatedQuestions || - !this.grapherState.relatedQuestions.length + this.hideRelatedQuestion || + !this.relatedQuestions || + !this.relatedQuestions.length ) return false - const question = this.grapherState.relatedQuestions[0] + const question = this.relatedQuestions[0] return !!question && !!question.text && !!question.url } @@ -3262,8 +3228,7 @@ export class Grapher extends React.Component { // "ourworldindata.org" and yet should still yield a match. // - Note that this won't work on production previews (where the // path is /admin/posts/preview/ID) - const { relatedQuestions = [] } = this.grapherState - const { hasRelatedQuestion } = this + const { relatedQuestions = [], hasRelatedQuestion } = this const relatedQuestion = relatedQuestions[0] return ( hasRelatedQuestion && @@ -3275,31 +3240,54 @@ export class Grapher extends React.Component { @computed get showRelatedQuestion(): boolean { return ( - !!this.grapherState.relatedQuestions && + !!this.relatedQuestions && !!this.hasRelatedQuestion && !!this.isRelatedQuestionTargetDifferentFromCurrentPage ) } @computed get isOnCanonicalUrl(): boolean { - if (!this.grapherState.canonicalUrl) return false + if (!this.canonicalUrl) return false return ( - getWindowUrl().pathname === - Url.fromURL(this.grapherState.canonicalUrl).pathname + getWindowUrl().pathname === Url.fromURL(this.canonicalUrl).pathname ) } @computed get showEntitySelectionToggle(): boolean { return ( - !this.grapherState.hideEntityControls && - this.grapherState.canChangeAddOrHighlightEntities && - this.grapherState.isOnChartTab && - (this.grapherState.showEntitySelectorAs === - GrapherWindowType.modal || - this.grapherState.showEntitySelectorAs === - GrapherWindowType.drawer) + !this.hideEntityControls && + this.canChangeAddOrHighlightEntities && + this.isOnChartTab && + (this.showEntitySelectorAs === GrapherWindowType.modal || + this.showEntitySelectorAs === GrapherWindowType.drawer) ) } +} + +export interface GrapherProps { + grapherState: GrapherState +} + +@observer +export class Grapher extends React.Component { + @computed get grapherState(): GrapherState { + return this.props.grapherState + } + + // #region Observable props not in any interface + + analytics = new GrapherAnalytics( + this.props.grapherState.initialOptions.env ?? "" + ) + + // stored on Grapher so state is preserved when switching to full-screen mode + + @observable + private legacyVariableDataJson?: MultipleOwidVariableDataDimensionsMap + private hasLoggedGAViewEvent = false + @observable private hasBeenVisible = false + @observable private uncaughtError?: Error + @observable slideShow?: SlideShowController constructor(props: { grapherState: GrapherState }) { super(props) @@ -3740,8 +3728,8 @@ export class Grapher extends React.Component { {/* captioned chart and entity selector */}
- {this.sidePanelBounds && ( - + {this.grapherState.sidePanelBounds && ( + )} @@ -3847,7 +3835,7 @@ export class Grapher extends React.Component { }, { threshold: [0, 0.66] } ) - observer.observe(this.containerElement!) + observer.observe(this.grapherState.containerElement!) this.grapherState.disposers.push(() => observer.disconnect()) } else { // IntersectionObserver not available; we may be in a Node environment, just render