Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔨 update grapher schema to latest version #3558

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

sophiamersmann
Copy link
Member

@sophiamersmann sophiamersmann commented Nov 18, 2024

Sibling PR to owid/owid-grapher#4159 that makes changes to the database and chart config schema.

Changes include:

  • Added derived column chart_configs.chartType
  • For chart configs:
    • Removed field type that is replaced by chartTypes
      • type used to refer to a single chart type, like "LineChart"
      • chartTypes refers to a list of chart types, like ["LineChart", "SlopeChart"]
      • For now, the only valid chart type combination is ["LineChart", "SlopeChart"]. We might add support for more chart type combinations in the future, but that will be a slow process. A lot of words to say: It is usually safe to assume the first element in chartTypes is the "type" of the chart
    • Removed field hasChartTab. If you want to know whether a Grapher has a chart tab, you can check whether chart_configs.chartType is null (if it is, then Grapher has no chart tab)

@owidbot
Copy link
Contributor

owidbot commented Nov 18, 2024

Quick links (staging server):

Site Dev Site Preview Admin Wizard Docs

Login: ssh owid@staging-site-update-grapher-schema-chart-types

chart-diff: ✅ No charts for review.

Edited: 2024-11-20 10:35:42 UTC
Execution time: 3.61 seconds

@Marigold
Copy link
Collaborator

@sophiamersmann happy to help you with ETL side if you want. Tests are currently failing because there are a couple of metadata files using hasChartTab: false which is not in the new schema. Since it's just three, it makes sense to fix the actual files rather than version them somehow.

@sophiamersmann sophiamersmann force-pushed the update-grapher-schema-chart-types branch 2 times, most recently from 1ebc563 to b35d9b9 Compare November 19, 2024 17:26
@sophiamersmann sophiamersmann force-pushed the update-grapher-schema-chart-types branch from b35d9b9 to a94c4cd Compare November 19, 2024 17:38
@sophiamersmann
Copy link
Member Author

@Marigold, thanks for looking into it. I think I got them all but one of the datasets doesn't compile. Do you mind taking over from here? I now added a description of what changed 👆

I can imagine that some of the data tools also refer to type somewhere and would need to be updated? Just a thought though (I really don't have a clue, lol)

@Marigold
Copy link
Collaborator

Sure, I'll take it from here. Just one thing - https://files.ourworldindata.org/schemas/grapher-schema.006.json returns 403. Is that expected?

@sophiamersmann
Copy link
Member Author

The new schema (006) will be uploaded automatically when the PR on the Grapher side is merged, but I could upload it earlier if you want me to?

@Marigold
Copy link
Collaborator

The new schema (006) will be uploaded automatically when the PR on the Grapher side is merged, but I could upload it earlier if you want me to?

The dataset is failing, because the schema isn't available at the URL. Which means it should work after we merge the Grapher side! (So don't worry about uploading it)

Copy link
Collaborator

@Marigold Marigold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! CI should be green after the Grapher part is merged.

@sophiamersmann sophiamersmann marked this pull request as ready for review November 20, 2024 16:14
sophiamersmann added a commit to owid/owid-grapher that referenced this pull request Nov 26, 2024
Adds support for multiple chart types in Grapher.

This PR adds general support for chart-type switching in Grapher but does not yet make it available in the admin or explorers since much more work is needed to make switching between line and slope charts actually a nice experience.

### Summary

- Config migration:
    - `type` -> `chartTypes`
    - `hasChartTab` has been removed
- Database changes:
    - Adds a `chart_configs.chartType` column. For map-only Graphers, `chartType` is null. For Graphers with multiple chart types, `chartType` is the first chart type in `chartTypes`
- Content switchers can show multiple chart tabs
- The selected chart type is serialised in the URL as `tab=line` or `tab=slope`. If more than one chart type is available, `tab=chart` selects the first given chart type.
- Support for the `hasChartTab` column in explorer configs has been dropped. If an author wants to hide the chart tab, they can select a special keyword ("None") for the `type` column. (No need to migrate anything since `hasChartTab` is not currently used by any published explorers.)
- Grapher is very strict about allowed chart type combinations (only line+slope charts are currently allowed) since it's not guaranteed that mixing different chart types works as expected
    - Line+slope charts are an easy case since we intend to make them work similarly. If we want to enable switching between line charts and scatters, for example, that would be more tricky since they need different data, the entity selector behaves differently, etc.

### Context

**Will be addressed in upcoming PRs:**
- Grapher uses enums for types, but enums don't compose well, which forced me to write crude assertions like `as unknown as ...`. The next PR refactors the types so that that isn't necessary anymore
- The bulk chart editor looks broken because it fetches the latest Grapher schema from DO, which has not yet been updated (it will be on merge). Beyond that, array fields are currently read-only, but we probably want the chart types to be editable. Both of these things will be addressed in a separate PR.
- We want to have a different icon for slope charts that look a little more like slope charts – Marwa is working on it

**Not addressed:**
- This PR doesn't guarantee that switching between any two chart types works as expected in Grapher. It provides the infrastructure, but we'll have to go through the code carefully when enabling chart type switching for more chart types.
- The way Grapher currently decides when/which labels to be hidden in the controls row so that content doesn't overflow is fragile. We should refactor this to be more robust, but for now, I quick-fixed it by updating media query thresholds so that it looks ok.

### Testing

I migrated the SVG tester configs locally to the new version, using `chartTypes` instead of `type`, and exported a new set of SVGs. No chart differences were found.

The Multiembedder fetches prod configs on staging, so any embedded chart (including all charts on test pages) are broken.

### Sibling PRs

- ETL: owid/etl#3558
- Datasette: owid/analytics#240
@sophiamersmann
Copy link
Member Author

@Marigold I just merged the PR on the Grapher side and then did another CI run. Building the staging env fails because there is a config that's apparently not valid (though I can't see why it wouldn't be valid). Is there something that needs to be fixed here?

@sophiamersmann sophiamersmann merged commit 8076d6b into master Nov 26, 2024
7 of 8 checks passed
@sophiamersmann sophiamersmann deleted the update-grapher-schema-chart-types branch November 26, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants