-
-
Notifications
You must be signed in to change notification settings - Fork 228
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
Explicitly pass client settings into Grapher #1602
Comments
Currently clientSettings contains:
Of these, Grapher uses Something to keep in mind 🤷 |
This issue has had no activity within 10 months. It is considered stale and will be closed in 7 days unless it is worked on or tagged as pinned. |
@marcelgerber have we done this as part of the vite changes? |
No, we didn't. In vite we're pretty much using the same settings flow as we had in webpack. |
This issue has had no activity within 10 months. It is considered stale and will be closed in 7 days unless it is worked on or tagged as pinned. |
Core problem
Client settings (defined in settings/clientSettings.ts are currently made available for Grapher by making webpack include the contents at build time. This has a number of downsides:
Proposed solution
Stop shipping clientsettings via webpack. Instead create a type for our settings and an instance with default values and extend the props of Grapher to take those, then pass them down to wherever they are used. We could also use context for this but less magic is probably better.
The Grapher instance would then in the constructor fill whatever is missing in the the passed settings with the default settings.
The default settings should maybe be baked into our standalone grapher pages. This would be weird on it's own but the approach some people from wikimedia have been taking would probably benefit from doing this (although we should check with them, maybe just having a settings object that is empty is good enough for them to work with.
One open question is what to do with constructed UI strings like "Add ENTITY" (where entity is replaced at runtime, usually with the label country) - but we can probably come up with a syntax for this.
Alternatives
Only do this for specific properties that need it (like I did in #1599 which was a motivating example for this change).
The text was updated successfully, but these errors were encountered: