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.

Sign up for GitHub

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

Jump to bottom

🔨 add empty arrays to the default grapher config #4294

Merged
merged 1 commit into from
Dec 17, 2024
Merged

🔨 add empty arrays to the default grapher config #4294

merged 1 commit into from
Dec 17, 2024

Conversation

sophiamersmann
Copy link
Member

@sophiamersmann sophiamersmann commented Dec 12, 2024 •
edited
Loading

Empty arrays are not currently included in Grapher's default config object that is derived from the schema. That's a bit of a foot gun, so I added them now.

For context: When I implemented inheritance, I decided to keep empty arrays out of the default config since all defaults ended up in the fullConfig of each chart and that would have been unnecessarily verbose. But now that we're not doing that anymore, I can't think of a downside of also including the default values for arrays.

This has two nice effects:

  • It used to be important that Grapher serialises empty arrays, so that a parent value can be overwritten with that empty array (e.g. if a parent has a related question that a child doesn't need). Now we can rely on these values being present in the default config.
  • Another nice effect is that we can simplify a number of tests like this one where selectedEntityNames was always serialised, even for an empty Grapher:

owid-grapher/packages/@ourworldindata/grapher/src/core/Grapher.jsdom.test.ts

Lines 92 to 98 in 28ba2e4

expect(new Grapher(input).toObject()).toEqual({
...input,
$schema: latestGrapherConfigSchema,
// TODO: ideally, selectedEntityNames is not serialised for an empty object
selectedEntityNames: [],
})

This was referenced Dec 12, 2024
Closed
Closed
Merged
Merged
Merged
Merged
@sophiamersmann Graphite App
Copy link
Member Author

sophiamersmann commented Dec 12, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Dec 12, 2024
Merged
Closed
@owidbot
Copy link
Contributor

owidbot commented Dec 12, 2024 •
edited
Loading

Quick links (staging server):

Site Dev Site Preview Admin Wizard Docs

Login: ssh owid@staging-site-default-config-empty-arrays-viz

SVG tester:

Number of differences (default views): 1357 (61b200) ❌
Number of differences (all views): 452 (7cdced) ❌

Edited: 2024-12-17 16:16:29 UTC
Execution time: 1.32 seconds

@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from 542d063 to 71cbc47 Compare December 13, 2024 13:15
@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from 71cbc47 to 3e1e8fe Compare December 13, 2024 13:20
@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from 3e1e8fe to 8906031 Compare December 13, 2024 14:02
@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch 2 times, most recently from 19e9eeb to 3cd081f Compare December 13, 2024 15:13
@sophiamersmann sophiamersmann marked this pull request as ready for review December 13, 2024 15:17
@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from 3cd081f to 2dfcaae Compare December 13, 2024 15:21
@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from 2dfcaae to dca509b Compare December 16, 2024 10:05
This was referenced Dec 16, 2024
Merged
Merged
marcelgerber
marcelgerber approved these changes Dec 16, 2024
View reviewed changes
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.

Nice! 🙌

@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from dca509b to 684cf54 Compare December 17, 2024 10:31
@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from 684cf54 to a1f8570 Compare December 17, 2024 15:34
@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from a1f8570 to 7db7446 Compare December 17, 2024 15:57
@sophiamersmann Graphite App
Copy link
Member Author

sophiamersmann commented Dec 17, 2024 •
edited
Loading

Merge activity

  • Dec 17, 11:23 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 17, 11:32 AM EST: Graphite rebased this pull request as part of a merge.
  • Dec 17, 11:33 AM EST: A user merged this pull request with Graphite.

sophiamersmann added a commit that referenced this pull request Dec 17, 2024
@sophiamersmann
🎉 (line+slope) add focus state / TAS-739 (#4272)
2c1bc83 
Adds the option to highlight lines in line and slope charts

### Summary

- Adds a new config field, `focusedSeriesNames` (suggestions for a different name welcome)
- A line is identified by its series name which is either an entity name, a column display name or a combination of both
- The list of focused series names is persisted to the URL as `focus` query param
    - The entity name utility functions are used to parse and serialise focused series names, so that the same delimiter is used and entity names are mapped to their codes if possible
    - This breaks if a column name contains `~` (the delimiter) which theoretically is possible but I don't think we need to worry about that now
- Focused lines have bold labels, non-focused lines are grayed out
- Grapher makes an effort to prevent the chart to enter a 'bad state' where all lines are grayed out because the focused line doesn't exist
    - This includes removing all elements from the focus array when the facet strategy changes and dismissing focused entities when they're unselected

#### In the admin

- There is a new 'Data to highlight' section below the entity selection section
- If the chart is in a bad state because one of the focused series names is invalid, saving is disabled and shows an error message

### Follow up

- It's a bit ugly that `selectedEntityNames` and `focusedSeriesNames` are always serialised, even for an empty Grapher. I've fixes that in a [follow-up PR](#4294)
- The line legend method that drops labels if there are to many is a bit difficult to read. I'll open another PR with a refactor
@sophiamersmann sophiamersmann changed the base branch from slope-figma-viz to graphite-base/4294 December 17, 2024 16:27
@sophiamersmann sophiamersmann changed the base branch from graphite-base/4294 to master December 17, 2024 16:29
@sophiamersmann sophiamersmann force-pushed the default-config-empty-arrays-viz branch from 7db7446 to 4d090e0 Compare December 17, 2024 16:31
@sophiamersmann sophiamersmann merged commit 0769513 into master Dec 17, 2024
14 of 16 checks passed
@sophiamersmann sophiamersmann deleted the default-config-empty-arrays-viz branch December 17, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelgerber marcelgerber marcelgerber approved these changes

Assignees

@sophiamersmann sophiamersmann

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sophiamersmann @owidbot @marcelgerber