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

Create shared config, layers and translations #1157

Merged
merged 26 commits into from
Apr 16, 2024
Merged

Conversation

ericboucher
Copy link
Collaborator

@ericboucher ericboucher commented Mar 14, 2024

Description

This fixes #358 by adding:

  • default config
  • shared layers
  • shared legends
  • shared translations

@wadhwamatic the configs are deeply merged, by default you have access to any layers in the shared layers and can just call them by id from your prism.json file as usual. If you need to tweak the name or the legends or else specifically for a country, you can do so by creating a layer with the same id with only the updated elements.

@wadhwamatic for now I simply created one shared/layers.json file but we can defiitely split it by products maybe for ease of use (no need to decide now).

How to test the feature:

  • make sure that translation works as expected
  • test moving some layer definitions to the shared space.

Checklist - did you ...

Test your changes with

  • REACT_APP_COUNTRY=rbd yarn start
  • REACT_APP_COUNTRY=cambodia yarn start
  • REACT_APP_COUNTRY=mozambique yarn start
  • Add / update necessary tests?
  • Add / update outdated documentation?

Screenshot/video of feature:

@ericboucher ericboucher mentioned this pull request Mar 14, 2024
5 tasks
Copy link

github-actions bot commented Mar 14, 2024

Build succeeded and deployed at https://prism-1157.surge.sh
(hash 7005c11 deployed at 2024-04-16T14:32:54)

@wadhwamatic
Copy link
Member

Thanks @ericboucher, this will be a big help when ready. A few things:

  1. The current layers.json in shared has errors. For example, the base_url and styles are incorrect
  2. What if some elements in the layer config are common, but others are not? For example, the levels property in chart_data is unique per country, but most other properties are common. We also would likely use one set of layer styles in more arid regions, and another in wetter regions. Would we need to then config those layers uniquely per country or could we find some way to inherit some properties, but not all?
  3. I'm thinking about how to deploy this. If prism.json refers to a layer ID that exists in both the local layers.json file, and the shared one, which will be used?

@ericboucher
Copy link
Collaborator Author

@wadhwamatic:
The current behavior is that local files overrides the shared one (deep merge) so:

  1. we can start from an empty file and you create the base layers that you want. As mentioned above, we could even split this into multiple files for clarity
  2. you can simply configure that specific parameter in the local config and let the rest come from the shared config
  3. both, with the local config taking precedent.

Here is an example:

shared file:
{url: hello, legend: a,b,c, map: {bbox: 1234}}

local file
{url: prism, map: {bbox: 4567}}

result:
{url: prism, legend: a,b,c, map: {bbox: 4567}}

@@ -1805,14 +1805,6 @@
},
"rainfall_agg_1month": {
Copy link
Collaborator Author

@ericboucher ericboucher Apr 12, 2024

Choose a reason for hiding this comment

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

@wadhwamatic you can even remove this from the layers.json altogether if there is no difference with what's in the shared file

Copy link
Member

Choose a reason for hiding this comment

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

@ericboucher - the title text is slightly different for Mozambique.

Copy link
Collaborator Author

@ericboucher ericboucher Apr 12, 2024

Choose a reason for hiding this comment

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

ok, does it need to be? you could just update the shared title maybe? Or is it not always CHIRPS?

Copy link
Member

Choose a reason for hiding this comment

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

We won't always include CHIRPS in the title. It's only for countries where we have local rainfall data and CHIRPS rainfall data.

@wadhwamatic
Copy link
Member

@ericboucher - before the shared layer concept, the order of layers in layers.json controlled the order of the 'select charts' dropdown list. Now, the shared layers are first, and then local layers. Can we reverse this? For Mozambique, I want the blended layers to show up first, then CHIRPS layers (which is how it worked before this change).

@wadhwamatic
Copy link
Member

@ericboucher - ok, I finished setting this up for Mozambique and I think it should be straightforward to apply to others. Making those configuration changes for all will take a while so I'd rather just do Mozambique in this one.

Two things:

  1. We need to update documentation to capture this change
  2. There's no longer a way to control layer order in charts. See my comment Create shared config, layers and translations #1157 (comment)

@ericboucher
Copy link
Collaborator Author

ericboucher commented Apr 14, 2024

@wadhwamatic I updated the readme, feel free to adjust as needed and merge when you think it's ready.

Two other changse we could make:

  • move the translation config to prism.json if we think that we can rely on one file per language in the future
  • create a shared prism.json file as well with default values and icons

Copy link
Collaborator

@echaidemenos echaidemenos left a comment

Choose a reason for hiding this comment

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

Everything looks and works OK for me here. Maybe we should split "General" and mozambique specific translations and use both mozambique/translations.json and shared/portuguese.json. Or just remove mozambique/translations.json for now

@ericboucher
Copy link
Collaborator Author

Yep or move mozambique/translations.json to mozambique/translations/portuguese.json or similar

@wadhwamatic
Copy link
Member

I deleted the local Mozambique translation file for now. If another Portuguese-speaking country comes along, we might need some level of localization for Moz vs a second country but for now will just keep a single shared Portuguese translation file.

@wadhwamatic wadhwamatic merged commit 2f461c3 into master Apr 16, 2024
6 checks passed
@wadhwamatic wadhwamatic deleted the shared-layers branch April 16, 2024 16:30
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.

Common configuration for WFP open data cube layers
3 participants