-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Conversation
Quick links (staging server):
Login: chart-diff: ✅No charts for review.Edited: 2024-11-20 10:35:42 UTC |
@sophiamersmann happy to help you with ETL side if you want. Tests are currently failing because there are a couple of metadata files using |
1ebc563
to
b35d9b9
Compare
b35d9b9
to
a94c4cd
Compare
@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 |
Sure, I'll take it from here. Just one thing - https://files.ourworldindata.org/schemas/grapher-schema.006.json returns 403. Is that expected? |
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) |
There was a problem hiding this 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.
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
@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? |
Sibling PR to owid/owid-grapher#4159 that makes changes to the database and chart config schema.
Changes include:
chart_configs.chartType
type
that is replaced bychartTypes
type
used to refer to a single chart type, like"LineChart"
chartTypes
refers to a list of chart types, like["LineChart", "SlopeChart"]
["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 inchartTypes
is the "type" of the charthasChartTab
. If you want to know whether a Grapher has a chart tab, you can check whetherchart_configs.chartType
is null (if it is, then Grapher has no chart tab)