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

Clean up languages/datasets for web and sanity #710

Closed
nilsml opened this issue Jan 7, 2022 · 6 comments
Closed

Clean up languages/datasets for web and sanity #710

nilsml opened this issue Jan 7, 2022 · 6 comments
Assignees
Labels
⚙️ task A single task. If linked to a user story it is a sub-task. 🛠 technical Technical stuffs like reducing debt, refactor or improve code base

Comments

@nilsml
Copy link
Contributor

nilsml commented Jan 7, 2022

TODO

  • Make a common type for the languages (we need to figure out how hard it is to share types across the web and sanity folders. This might be to complicated for this issue)

  • Avoid to use let in the languages file (in the web). Or at least make it easier to see what is going on here

  • Use less hard coded string values for datasets (I had to change from production too many places, ie in DesktopStructure.js

  • We should add the base language explicit (for use in the document translation feature from the intl plugin). Both for the web part (next.config.js) and the documentTranslation (intl-plugin) config in Sanity

@nilsml nilsml added ⚙️ task A single task. If linked to a user story it is a sub-task. 🛠 technical Technical stuffs like reducing debt, refactor or improve code base labels Jan 7, 2022
@fernandolucchesi fernandolucchesi self-assigned this Jan 11, 2022
@nilsml
Copy link
Contributor Author

nilsml commented Jan 12, 2022

Since we do not yet share code between web and sanity, I suggest creating types for each now, and then we can see if we should consolidate the two at some point in the future. We will have to do changes two places, but I guess it is manageable. The important part is to get rid of magic strings.

@fernandolucchesi
Copy link
Contributor

With the new approach all the languages/dataset setup is done throught the file satellitesConfig.js located in the root folder.

image

The base language is still being defined accordingly to the order of the language array (check comment on line 8).

I believe this to be a clean and simple solution and don't see the need of making it more complex than that, but happy to refactor if the team disagrees.

I also moved the flags from the array to inside the studio, since they are only used there. So it is nice (but not needed), to add the country flag when adding a new language. Setup is done through the file studio/icons/countries/index.ts

image

GOTCHAS:
I couldn't use es6 modules for the languages.js files and satellitesConfig.js file. Could make it run locally but crashing when building next.

We can't use typescript since the file is read by next.config.mjs, and next currently doesn't accept ts in this file

@wenche
Copy link
Contributor

wenche commented Jan 17, 2022

Pros

Easier to read the code
Fewer occurrences of magic strings, at least the one we need to change when we use another dataset
More flexible base language
Satellite config is shared between the web and the studio

Cons

Lack of typescript because of Next config, but I don't think that will change.
Lack of ES6 modules, we should probably create an issue in the backlog and look more closely into this at some point?
We still need both en_GB and en-GB, do you remember the exact reason? The sanity intl plugin?
global is still hard coded several places as a way to differentiate between the main Equinor homepage and satellites. Would it make sense to export this as a constant or something?

We should probably not spend to much time on this 😅

What do you think @nilsml ?

@wenche
Copy link
Contributor

wenche commented Jan 17, 2022

I tried to explain things slightly more verbose in the satellites config, but I'm actually not sure that it's 100% correct

@wenche
Copy link
Contributor

wenche commented Jan 17, 2022

...and the plot thickens

The new version of the Sanity language plugin has changed the config. Name is now id and they are using - as a delimiter for language and some config changes in general

@nilsml
Copy link
Contributor Author

nilsml commented Jan 18, 2022

I did a review of the code. It is definitely better than what it was, but still room for improvements. Either by creating a DSL for the config or use functional concepts, we could make it less error-prone, but at the same time we need to move on. I will create a task in the backlog for further improvements #745

I reckon this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ task A single task. If linked to a user story it is a sub-task. 🛠 technical Technical stuffs like reducing debt, refactor or improve code base
Projects
None yet
Development

No branches or pull requests

4 participants