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

Activer ou désactiver les analytics #272

Merged
merged 1 commit into from
Jun 4, 2021
Merged

Conversation

octo-topi
Copy link
Contributor

🦄 Problème

Le serveur d'analytics peut être indisponible, et nosu souhtatiosn dans ce cas en plus l'alimenter

🤖 Solution

Conditionner l'utilisation à la variable MATOMO_URL

🌈 Remarques

Ajout sample.env pour documenter

💯 Pour tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc.

@pix-service
Copy link

I'm deploying this PR to these urls:

Please check it out!

@octo-topi octo-topi force-pushed the tech-toggle-analytics branch from 4c72783 to c9e1909 Compare June 4, 2021 14:15
@octo-topi octo-topi merged commit bfaac70 into dev Jun 4, 2021
@octo-topi octo-topi deleted the tech-toggle-analytics branch June 4, 2021 14:21
Comment on lines +205 to +213

if (process.env.MATOMO_URL) {
config.modules.push([
'nuxt-matomo',
{ matomoUrl: process.env.MATOMO_URL, siteId: 1 },
])
}

export default config
Copy link
Member

Choose a reason for hiding this comment

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

Je suis pas fan, je dois penser à regarder en bas du fichier si c'est bien ajouté ou non :/

Au lieu de tout ça cela aurait pu être :

modules: [
   // Doc: https://axios.nuxtjs.org/usage
  '@nuxtjs/axios',
  '@nuxtjs/dotenv',
  '@nuxtjs/style-resources',   
['nuxt-i18n', { detectBrowserLanguage: false }],
 '@nuxtjs/moment',
  ...(process.env.MATOMO_URL ? ['nuxt-matomo', { matomoUrl: process.env.MATOMO_URL, siteId: 1 }] : []) 
]

Voir :

modules: [
   // Doc: https://axios.nuxtjs.org/usage
  '@nuxtjs/axios',
  '@nuxtjs/dotenv',
  '@nuxtjs/style-resources',   
['nuxt-i18n', { detectBrowserLanguage: false }],
 '@nuxtjs/moment',
  ...(_addMatomoAnalyticsIfEnabled()) 
]


function _addMatomoAnalyticsIfEnabled() {
  return process.env.MATOMO_URL ? ['nuxt-matomo', { matomoUrl: process.env.MATOMO_URL, siteId: 1 }] : [];
}

Copy link

Choose a reason for hiding this comment

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

Je sais pas trop si jamais je veux savoir si Matomo est utilisé ou pas je sais que je vais faire une recherche avec MATOMO_URL et je vais tombé dessus aussi vite qu'avec la version avec la ternaire.

Je trouve que la version avec la ternaire et la destructuration moins évidente à lire et l'intention ne saute pas au yeux.

C'est purement subjectif mais pour moi les ternaires c'est rarement une bonne solution parce que ça doit rester "simple" et c'est rarement le cas. Ici la ternaire fait apparaître une petite complexité lié à l'utilisation d'un tableau vide quand on désactive Matomo.
De mon point de vue moi c'était et c'est toujours pas évident à la lecture que la destructuration de ...([]) ne renvoie rien. Cette petite complexité réduit le nombre de ligne de code, mais ne rend pas l'intention plus claire pour moi.

Tout ça pour dire que je suis pas fan de la ternaire, mais je suis ok pour valider la PR si jamais tu veux changer l'implémentation 😉.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants