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

[TECH] Simplifier la lecture des configurations i18n en fonction du domaine. #440

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

VincentHardouin
Copy link
Member

@VincentHardouin VincentHardouin commented Dec 15, 2022

🦄 Problème

Lire la configuration i18n en fonction du domaine sur lequel on se trouve peut être complexe.

🤖 Solution

Exporter les configurations liées au domaine dans des constantes dédiées pour faciliter la lecture

🌈 Remarques

On peut se demander si on souhaite pas extraire même la configuration en commun dans chaque nouvelle variable.

Au passage, j'ai remis dans la config le service is-seo-indexing-enabled qui est l'endroit le plus adapté

💯 Pour tester

  • Vérifier le bon fonctionnement des 4 RA

@pix-service
Copy link

I'm deploying this PR to these urls:

Please check it out!

@VincentHardouin VincentHardouin added the team-evaluation PR relatives à l'expérience d'évaluation label Dec 15, 2022
Copy link
Contributor

@lego-technix lego-technix left a comment

Choose a reason for hiding this comment

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

Effectivement, la lecture est plus aisée, et J'ai vérifié fonctionnellement qu'il n'y avait pas de régression.

import { language } from './config/language'
import { config } from './config/environment'
import { SITES_PRISMIC_TAGS } from './services/available-sites'

const i18nConfigurationForFrenchDomain = {
defaultLocale: 'fr-fr',
Copy link
Contributor

Choose a reason for hiding this comment

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

question : on attend que l'ADR soit validé pour changer la syntaxe des locales en 'fr-FR' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bonne question : je pense que cette PR a pour but de préparer le terrain avant de faire les modifs importantes dans les meilleures conditions possibles : modification de la 1ère page, et prise en compte plus ou moins complète de l'ADR.

nuxt.config.js Outdated
vueI18n: {
fallbackLocale: config.isFrenchDomain ? 'fr-fr' : 'fr',
},
...(config.isFrenchDomain
Copy link
Contributor

Choose a reason for hiding this comment

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

question : pourquoi on définit seulement les cas en français ?

Copy link
Member

@yannbertrand yannbertrand Dec 16, 2022

Choose a reason for hiding this comment

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

Car il n'y a que sur *.pix.fr que la configuration est différente de *.pix.org. Si on avait d'autres domaines à gérer, il faudrait probablement générer une config pour chacun d'entre eux 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

D'accord merci ! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 Ready to Merge team-evaluation PR relatives à l'expérience d'évaluation Tech Review OK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants