Skip to content

Commit

Permalink
🔨 (grapher) replace some enum types / TAS-709 (#4170)
Browse files Browse the repository at this point in the history
Refactors Grapher types by replacing enums with compile-time-only types.

I introduced `GrapherTabName` in the previous PR, which is either a `ChartTypeName` or `"WorldMap"` or `"Table"`. But enums currently [can't be extended or merged](microsoft/TypeScript#17592), which means that I had to copy-paste all chart type names into `GrapherTabName`. As a result, TypeScript doesn't do a good job of type narrowing, forcing me to write (unnecessary) type assertions in many places.

This PR refactors `ChartTypeName` and other tab-related types so they don't use enums. However, I still wanted the convenience of a single object to access chart types, for example. To get that, the types are inferred from const objects:

```ts
const GRAPHER_CHART_TYPES = {
    LineChart: "LineChart",
    ScatterPlot: "ScatterPlot"
} as const

type GrapherChartType = keyof typeof GRAPHER_CHART_TYPES
```

I first did it the other way around (first defining the types, then using them for the objects), but it turns out the way it's done now leads to better type narrowing. 

**Newly introduced types:**
- `GrapherMapType`: only "WorldMap"
- `GrapherChartType`: "LineChart", "ScatterPlot", etc. (without "WorldMap")
- `GrapherChartOrMapType`: `GrapherChartType` or `GrapherMapType`
- `GrapherTabName`: Internal tab names used in Grapher (a chart type name or "WorldMap" or "Table")
- `GrapherTabOption`: Grapher tab specified in the config that determines the default tab to show ("chart", "map" or "tab")
- `GrapherTabQueryParam`: Valid values for the `tab` query parameter in Grapher ("chart", "map", "line", "slope", etc.)
  • Loading branch information
sophiamersmann authored Nov 26, 2024
1 parent cfe0245 commit c77e1cf
Show file tree
Hide file tree
Showing 60 changed files with 593 additions and 514 deletions.
13 changes: 7 additions & 6 deletions adminSiteClient/ChartList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import { runInAction, observable } from "mobx"
import { bind } from "decko"
import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js"
import {
ChartTypeName,
GrapherChartType,
GrapherInterface,
GrapherTabOption,
GRAPHER_CHART_TYPES,
GRAPHER_TAB_OPTIONS,
} from "@ourworldindata/types"
import { startCase, DbChartTagJoin } from "@ourworldindata/utils"
import { References, getFullReferencesCount } from "./ChartEditor.js"
Expand All @@ -24,7 +25,7 @@ export interface ChartListItem {
tab: GrapherInterface["tab"]
hasMapTab: GrapherInterface["hasMapTab"]

type?: ChartTypeName
type?: GrapherChartType
hasChartTab: boolean

lastEditedAt: string
Expand Down Expand Up @@ -152,11 +153,11 @@ export function showChartType(chart: ChartListItem): string {

if (!chartType) return "Map"

const displayType = ChartTypeName[chartType]
? startCase(ChartTypeName[chartType])
const displayType = GRAPHER_CHART_TYPES[chartType]
? startCase(GRAPHER_CHART_TYPES[chartType])
: "Unknown"

if (chart.tab === GrapherTabOption.map) {
if (chart.tab === GRAPHER_TAB_OPTIONS.map) {
if (chart.hasChartTab) return `Map + ${displayType}`
else return "Map"
} else {
Expand Down
4 changes: 2 additions & 2 deletions adminSiteClient/ColorSchemeDropdown.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React from "react"
import { computed, action } from "mobx"
import Select from "react-select"
import { ChartTypeName } from "@ourworldindata/types"
import { GrapherChartOrMapType } from "@ourworldindata/types"
import {
ColorScheme,
getColorSchemeForChartType,
Expand All @@ -21,7 +21,7 @@ interface ColorSchemeDropdownProps {
value?: string
gradientColorCount: number
invertedColorScheme: boolean
chartType: ChartTypeName
chartType: GrapherChartOrMapType
onChange: (selected: ColorSchemeOption) => void
onBlur?: () => void
}
Expand Down
13 changes: 6 additions & 7 deletions adminSiteClient/EditorBasicTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ import {
} from "mobx"
import { observer } from "mobx-react"
import {
ChartTypeName,
EntitySelectionMode,
StackMode,
ALL_GRAPHER_CHART_TYPES,
GrapherChartType,
} from "@ourworldindata/types"
import {
DimensionSlot,
Expand Down Expand Up @@ -362,7 +363,9 @@ export class EditorBasicTab<
const { grapher } = this.props.editor

grapher.chartTypes =
value === this.chartTypeOptionNone ? [] : [value as ChartTypeName]
value === this.chartTypeOptionNone
? []
: [value as GrapherChartType]

if (grapher.isMarimekko) {
grapher.hideRelativeToggle = false
Expand Down Expand Up @@ -403,11 +406,7 @@ export class EditorBasicTab<
value: string
label: string
}[] {
const allChartTypes = Object.keys(ChartTypeName).filter(
(chartType) => chartType !== ChartTypeName.WorldMap
)

const chartTypeOptions = allChartTypes.map((key) => ({
const chartTypeOptions = ALL_GRAPHER_CHART_TYPES.map((key) => ({
value: key,
label: startCase(key),
}))
Expand Down
6 changes: 3 additions & 3 deletions adminSiteClient/EditorColorScaleSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { faPlus, faMinus } from "@fortawesome/free-solid-svg-icons"
import {
ColorSchemeName,
BinningStrategy,
ChartTypeName,
GrapherChartOrMapType,
Color,
} from "@ourworldindata/types"
import {
Expand Down Expand Up @@ -43,7 +43,7 @@ interface EditorColorScaleSectionFeatures {
@observer
export class EditorColorScaleSection extends React.Component<{
scale: ColorScale
chartType: ChartTypeName
chartType: GrapherChartOrMapType
features: EditorColorScaleSectionFeatures
showLineChartColors: boolean
onChange?: () => void
Expand Down Expand Up @@ -132,7 +132,7 @@ class ColorLegendSection extends React.Component<{
@observer
class ColorsSection extends React.Component<{
scale: ColorScale
chartType: ChartTypeName
chartType: GrapherChartOrMapType
showLineChartColors: boolean
onChange?: () => void
}> {
Expand Down
8 changes: 5 additions & 3 deletions adminSiteClient/EditorCustomizeTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
ColorSchemeName,
FacetAxisDomain,
FacetStrategy,
ChartTypeName,
GRAPHER_CHART_TYPES,
} from "@ourworldindata/types"
import { Grapher } from "@ourworldindata/grapher"
import {
Expand Down Expand Up @@ -161,7 +161,7 @@ export class ColorSchemeSelector extends React.Component<{
onBlur={this.onBlur}
chartType={
this.props.grapher.chartType ??
ChartTypeName.LineChart
GRAPHER_CHART_TYPES.LineChart
}
invertedColorScheme={!!grapher.invertColorScheme}
additionalOptions={[
Expand Down Expand Up @@ -755,7 +755,9 @@ export class EditorCustomizeTab<
{grapher.chartInstanceExceptMap.colorScale && (
<EditorColorScaleSection
scale={grapher.chartInstanceExceptMap.colorScale}
chartType={grapher.chartType ?? ChartTypeName.LineChart}
chartType={
grapher.chartType ?? GRAPHER_CHART_TYPES.LineChart
}
showLineChartColors={grapher.isLineChart}
features={{
visualScaling: true,
Expand Down
4 changes: 2 additions & 2 deletions adminSiteClient/EditorMapTab.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
ChartTypeName,
GrapherInterface,
MapProjectionName,
GRAPHER_MAP_TYPE,
} from "@ourworldindata/types"
import {
ChartDimension,
Expand Down Expand Up @@ -202,7 +202,7 @@ export class EditorMapTab<
<TimelineSection mapConfig={mapConfig} />
<EditorColorScaleSection
scale={colorScale}
chartType={ChartTypeName.WorldMap}
chartType={GRAPHER_MAP_TYPE}
showLineChartColors={false}
features={{
visualScaling: true,
Expand Down
7 changes: 4 additions & 3 deletions adminSiteClient/GrapherConfigGridEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import { BaseEditorComponent, HotColumn, HotTable } from "@handsontable/react"
import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js"

import Handsontable from "handsontable"
import { ChartTypeName } from "@ourworldindata/types"
import { GRAPHER_CHART_TYPES, GRAPHER_MAP_TYPE } from "@ourworldindata/types"
import {
Grapher,
GrapherProgrammaticInterface,
Expand Down Expand Up @@ -488,7 +488,7 @@ export class GrapherConfigGridEditor extends React.Component<GrapherConfigGridEd
return colorScale ? (
<EditorColorScaleSection
scale={colorScale}
chartType={ChartTypeName.WorldMap}
chartType={GRAPHER_MAP_TYPE}
features={{
visualScaling: true,
legendDescription: false,
Expand All @@ -513,7 +513,8 @@ export class GrapherConfigGridEditor extends React.Component<GrapherConfigGridEd
<EditorColorScaleSection
scale={colorScale}
chartType={
grapher.chartType ?? ChartTypeName.LineChart
grapher.chartType ??
GRAPHER_CHART_TYPES.LineChart
}
features={{
visualScaling: true,
Expand Down
6 changes: 3 additions & 3 deletions adminSiteClient/VariableEditPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import { SourceList } from "./SourceList.js"
import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js"
import { Base64 } from "js-base64"
import {
GrapherTabOption,
GRAPHER_TAB_OPTIONS,
GrapherInterface,
OwidVariableRoundingMode,
} from "@ourworldindata/types"
Expand Down Expand Up @@ -691,13 +691,13 @@ class VariableEditor extends React.Component<{
return {
...grapherConfig,
hasMapTab: true,
tab: GrapherTabOption.map,
tab: GRAPHER_TAB_OPTIONS.map,
}
else
return {
yAxis: { min: 0 },
map: { columnSlug: this.props.variable.id.toString() },
tab: GrapherTabOption.map,
tab: GRAPHER_TAB_OPTIONS.map,
hasMapTab: true,
dimensions: [
{
Expand Down
28 changes: 15 additions & 13 deletions adminSiteServer/testPageRouter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,16 @@ import {
} from "@ourworldindata/utils"
import { grapherToSVG } from "../baker/GrapherImageBaker.js"
import {
ChartTypeName,
ColorSchemeName,
DbRawChartConfig,
DbPlainChart,
EntitySelectionMode,
GrapherTabOption,
GRAPHER_TAB_OPTIONS,
StackMode,
parseChartConfig,
GRAPHER_MAP_TYPE,
GrapherTabOption,
GrapherChartOrMapType,
} from "@ourworldindata/types"
import { ExplorerAdminServer } from "../explorerAdminServer/ExplorerAdminServer.js"
import { GIT_CMS_DIR } from "../gitCms/GitCmsConstants.js"
Expand Down Expand Up @@ -101,7 +103,7 @@ interface EmbedTestPageQueryParams {
readonly page?: string
readonly random?: string
readonly tab?: GrapherTabOption
readonly type?: ChartTypeName
readonly type?: GrapherChartOrMapType
readonly logLinear?: string
readonly comparisonLines?: string
readonly stackMode?: StackMode
Expand Down Expand Up @@ -148,41 +150,41 @@ async function propsFromQueryParams(
let tab = params.tab

if (params.type) {
if (params.type === ChartTypeName.WorldMap) {
if (params.type === GRAPHER_MAP_TYPE) {
query = query.andWhereRaw(`cc.full->>"$.hasMapTab" = "true"`)
tab = tab || GrapherTabOption.map
tab = tab || GRAPHER_TAB_OPTIONS.map
} else {
query = query.andWhereRaw(`cc.chartType = :type`, {
type: params.type,
})
tab = tab || GrapherTabOption.chart
tab = tab || GRAPHER_TAB_OPTIONS.chart
}
}

if (params.logLinear) {
query = query.andWhereRaw(
`cc.full->>'$.yAxis.canChangeScaleType' = "true" OR cc.full->>'$.xAxis.canChangeScaleType' = "true"`
)
tab = GrapherTabOption.chart
tab = GRAPHER_TAB_OPTIONS.chart
}

if (params.comparisonLines) {
query = query.andWhereRaw(
`cc.full->'$.comparisonLines[0].yEquals' != ''`
)
tab = GrapherTabOption.chart
tab = GRAPHER_TAB_OPTIONS.chart
}

if (params.stackMode) {
query = query.andWhereRaw(`cc.full->'$.stackMode' = :stackMode`, {
stackMode: params.stackMode,
})
tab = GrapherTabOption.chart
tab = GRAPHER_TAB_OPTIONS.chart
}

if (params.relativeToggle) {
query = query.andWhereRaw(`cc.full->>'$.hideRelativeToggle' = "false"`)
tab = GrapherTabOption.chart
tab = GRAPHER_TAB_OPTIONS.chart
}

if (params.categoricalLegend) {
Expand All @@ -192,7 +194,7 @@ async function propsFromQueryParams(
query = query.andWhereRaw(
`json_length(cc.full->'$.map.colorScale.customCategoryColors') > 1`
)
tab = GrapherTabOption.map
tab = GRAPHER_TAB_OPTIONS.map
}

if (params.mixedTimeTypes) {
Expand Down Expand Up @@ -232,9 +234,9 @@ async function propsFromQueryParams(
query = query.andWhereRaw(`charts.id IN (${params.ids})`)
}

if (tab === GrapherTabOption.map) {
if (tab === GRAPHER_TAB_OPTIONS.map) {
query = query.andWhereRaw(`cc.full->>"$.hasMapTab" = "true"`)
} else if (tab === GrapherTabOption.chart) {
} else if (tab === GRAPHER_TAB_OPTIONS.chart) {
query = query.andWhereRaw(`cc.chartType IS NOT NULL`)
}

Expand Down
6 changes: 3 additions & 3 deletions baker/updateChartEntities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
ChartsXEntitiesTableName,
DbPlainChart,
GrapherInterface,
GrapherTabOption,
GRAPHER_TAB_OPTIONS,
MultipleOwidVariableDataDimensionsMap,
OwidVariableDataMetadataDimensions,
DbRawChartConfig,
Expand Down Expand Up @@ -97,7 +97,7 @@ const obtainAvailableEntitiesForGrapherConfig = async (

// If the grapher has a chart tab, then the available entities there are the "most interesting" ones to us
if (grapher.hasChartTab) {
grapher.tab = GrapherTabOption.chart
grapher.tab = GRAPHER_TAB_OPTIONS.chart

// If the grapher allows for changing or multi-selecting entities, then let's index all entities the
// user can choose from. Otherwise, we'll just use the default-selected entities.
Expand All @@ -112,7 +112,7 @@ const obtainAvailableEntitiesForGrapherConfig = async (
return grapher.tableForSelection.availableEntityNames as string[]
else return grapher.selectedEntityNames ?? []
} else if (grapher.hasMapTab) {
grapher.tab = GrapherTabOption.map
grapher.tab = GRAPHER_TAB_OPTIONS.map
// On a map tab, tableAfterAuthorTimelineAndActiveChartTransform contains all
// mappable entities for which data is available
return grapher.tableAfterAuthorTimelineAndActiveChartTransform
Expand Down
12 changes: 6 additions & 6 deletions db/migration/1661264304751-MigrateSelectedData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { MigrationInterface, QueryRunner } from "typeorm"

import { entityNameById } from "./data/entityNameById.js"

import { ChartTypeName } from "@ourworldindata/types"
import { GRAPHER_CHART_TYPES, GrapherChartType } from "@ourworldindata/types"

type GrapherInterface = Record<string, any>

Expand Down Expand Up @@ -85,11 +85,11 @@ export class MigrateSelectedData1661264304751 implements MigrationInterface {
}
})

const migrateDimensionsTypes: ChartTypeName[] = [
ChartTypeName.Marimekko,
ChartTypeName.StackedArea,
ChartTypeName.StackedBar,
ChartTypeName.StackedDiscreteBar,
const migrateDimensionsTypes: GrapherChartType[] = [
GRAPHER_CHART_TYPES.Marimekko,
GRAPHER_CHART_TYPES.StackedArea,
GRAPHER_CHART_TYPES.StackedBar,
GRAPHER_CHART_TYPES.StackedDiscreteBar,
]

// Migrate order of dimensions.
Expand Down
4 changes: 2 additions & 2 deletions db/model/Chart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
} from "@ourworldindata/utils"
import {
GrapherInterface,
ChartTypeName,
RelatedChart,
DbPlainPostLink,
DbPlainChart,
Expand All @@ -22,6 +21,7 @@ import {
DbPlainTag,
DbRawChartConfig,
DbEnrichedChartConfig,
GrapherChartType,
} from "@ourworldindata/types"
import { OpenAI } from "openai"
import {
Expand Down Expand Up @@ -556,7 +556,7 @@ export async function getChartVariableData(

export const getMostViewedGrapherIdsByChartType = async (
knex: db.KnexReadonlyTransaction,
chartType: ChartTypeName,
chartType: GrapherChartType,
count = 10
): Promise<number[]> => {
const ids = await db.knexRaw<{ id: number }>(
Expand Down
Loading

0 comments on commit c77e1cf

Please sign in to comment.