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

🎉 (grapher) support multiple chart types / TAS-694 #4159

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

sophiamersmann
Copy link
Member

@sophiamersmann sophiamersmann commented Nov 13, 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

@sophiamersmann sophiamersmann changed the title ✨ (grapher) support multiple chart types ✨ (grapher) support multiple chart types / TAS-694 Nov 13, 2024
Copy link

@owidbot
Copy link
Contributor

owidbot commented Nov 13, 2024

Quick links (staging server):

Site Admin Wizard Docs

Login: ssh owid@staging-site-grapher-types-viz

SVG tester:

Number of differences (default views): 1366 (41e0ea) ❌
Number of differences (all views): 0 ✅

Edited: 2024-11-26 09:45:26 UTC
Execution time: 1.25 seconds

@sophiamersmann sophiamersmann force-pushed the grapher-types-viz branch 3 times, most recently from 7903962 to b29748a Compare November 18, 2024 15:16
@sophiamersmann sophiamersmann marked this pull request as ready for review November 18, 2024 15:35
@sophiamersmann sophiamersmann force-pushed the grapher-types-viz branch 2 times, most recently from 96b4143 to 1fa7721 Compare November 19, 2024 16:59
@sophiamersmann sophiamersmann force-pushed the grapher-types-viz branch 4 times, most recently from 4367236 to 74039fd Compare November 20, 2024 14:14
@sophiamersmann sophiamersmann force-pushed the grapher-types-viz branch 5 times, most recently from 14ea177 to 48abe5f Compare November 21, 2024 12:57
@sophiamersmann sophiamersmann changed the title ✨ (grapher) support multiple chart types / TAS-694 🎉 (grapher) support multiple chart types / TAS-694 Nov 22, 2024
Copy link
Member

@marcelgerber marcelgerber left a comment

Choose a reason for hiding this comment

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

This is really well done, awesome!
I tested this a bunch by running the migration locally, and looking at a bunch of charts and setting one chart to line+slope. Didn't notice anything odd.

db/migration/1731431457062-AddTypesFieldToConfigs.ts Outdated Show resolved Hide resolved
adminSiteServer/testPageRouter.tsx Outdated Show resolved Hide resolved
@sophiamersmann sophiamersmann force-pushed the grapher-types-viz branch 2 times, most recently from 01171a2 to 833685e Compare November 26, 2024 09:09
Copy link
Member Author

sophiamersmann commented Nov 26, 2024

Merge activity

  • Nov 26, 4:52 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 26, 4:52 AM EST: A user merged this pull request with Graphite.

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