-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
🔨 Fix grapher uses after grapher state refactor [4/4] #4419
base: master
Are you sure you want to change the base?
Conversation
b7638b8
to
e1336f6
Compare
70ad31c
to
2348bca
Compare
41c0837
to
e28bfc1
Compare
2348bca
to
e22a5b5
Compare
e28bfc1
to
65c5675
Compare
e22a5b5
to
d4e1269
Compare
faf3b7c
to
4325f5c
Compare
530ed25
to
f97f5e8
Compare
5c00160
to
2abf5d5
Compare
f97f5e8
to
954bcac
Compare
2d53d5a
to
9d1ae90
Compare
954bcac
to
f859fe0
Compare
9d1ae90
to
04e15bc
Compare
f859fe0
to
ab5227e
Compare
9358595
to
1f83a0a
Compare
1f83a0a
to
28db21a
Compare
26357c4
to
7709aec
Compare
9619290
to
dff2264
Compare
grapher.seriesColorMap?.clear() | ||
grapher.rebuildInputOwidTable() | ||
grapherState.seriesColorMap?.clear() | ||
// TODO: 2025-01-05 Daniel we used to rebuild the table here but that |
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.
A new Todo was discovered. If it is not a priority right now,consider marking it for later attention.
TODO: 2025-01-05 Daniel we used to rebuild the table here but that
- Ignore
fetchConfigForSlug?: boolean | ||
} | ||
|
||
// TODO: change this so it's possible to hand a full grapher config down (and maybe an extra config?) |
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.
A new Todo was discovered. If it is not a priority right now,consider marking it for later attention.
TODO: change this so it's possible to hand a full grapher config down (and maybe an extra config?)
- Ignore
a50afba
to
2a94e58
Compare
2a94e58
to
1dc6512
Compare
This is the second big PR in the stack, the one that tries to fix things again. This PR attempts to make Grapher useable again in all places after the state was extracted into
GrapherState
and the automatic data loading was taken out in favour of explicit functions for loading and preparing the input table.Most of the internal MobX logic that was in
Grapher
is now extracted into a utity classGrapherState
. Grapher takes aGrapherState
instance as it's only prop - the idea is that react code at some point creates a newGrapherState
and keeps it around in a ref and just passes that one as a prop to concreteGrapher
components. Eventually,Grapher
might even just be a functional component, but there are still a few properties and actions and so on on theGrapher
class in this PR.The
GrapherState
instance is also what is passed as manager into various other components to control aspects of them and it is what is now used in the Grapher tests.The various chart instances (LineChart, ...) are not yet separated from their state. I think it would be nice to give them the same treatment that Grapher got (extract all their mobx state into a utility class and make the actual React component very lightweight) but this PR is already so huge...
As for using Grapher on our site, the basic idea with this PR is to have the following component hierarchy (from most comfortable to use to most basic one):
GrapherWithFallback
- shows aGrapherImage
thumbnail on server render, on client render uses an interaction observer to initialize aGrapherFigureView
with aGrapherImage
as fallback.GrapherFigureView
- measures the size of the containing element and updatedFetchingGrapher
accordinglyFetchingGrapher
- takes care of loading variable data and metadata files and initializing the InputTableGrapher
- just theGrapher
that doesn't load data. It needs theinputTable
to be set from an external user.One spiritual goal of this PR stack is to get rid of the
Multiembedder
and just useGrapherWithFallback
to take care of the interaction observers and so on. This PR takes a first step into that direction in that theChart
gdoc component now uses aGrapherWithFallback
instead of rendering a figure that is then replaced by the Multiembedder, but it does not do this for a bunch of other cases (e.g. explorers, Mdims, ...) or on pages that are not yet properly hydrated like the old standalone Grapher pages.Known blockers
(please add more as you find them)
owidDataset
isn't properly loaded inSampleExplorerOfGraphers
)Chart
component can have customizations (through the query string) that are not yet passed down through the updated component hierarchy.Open questions / things to check
GrapherWithFallback
, ...) changed quite a bit, there might be a lot of remaining smaller issues around the site when Grapher is used.authorsVersion
andinitialOptions
do almost the same thing now - can they be unified?Steps to discuss for after this is merged
GrapherProgrammaticInterface
and then all the properties computed from that. Is it good enough if we rely on the type system to hide implementation detail fields of GrapherState or would we prefer to have one class that is the grapher API that can be manipulated and another one that then derives all the other state from that?