fix(tabs): init tabs with custom active index other than zero #677

remija · 2020-09-09T08:20:37Z

Related issue

Unable to activate index upper than zero (default) when init Tabs component

Reference to the issue

Here you can add link to related issue

Description of the issue

It is impossible to display particular tab other than 0 index when showing Tabs component

Person(s) for reviewing proposed changes

Important

Before creating a pull request run unit tests

$ npm test

# watch for changes
$ npm test -- --watch

# For a specific file (e.g., in packages/context/__tests__/command.test.js)
$ npm test -- --watch packages/action

sisko59 · 2020-09-09T12:58:00Z

Félicitations pour cette première PR 🥇

romuleald · 2020-09-09T14:10:21Z

packages/help/src/__snapshots__/Help.spec.js.snap

@@ -1,12 +1,14 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

 exports[`<Help> Render Should render Help component 1`] = `
-<button
+<div


Question : Comment ça se fait que l'on passe sur un div ici ?

Honnetement je ne sais pas car je n'ai pas modifié ce composant :) J'ai juste remarqué qu'un TU sur celui-ci ne passait pas et j'ai juste mis à jour le snapshot (Il parait qu'il y a une autre PR à ce sujet).

Oui, il y a un problème actuellement avec ce test, qui fonctionne différemment entre nos poste et la build.
De mon coté pour que ma PR build, j'ai forcé exceptionnellement mon push sans la mise à jour de la snapshot car ça ne me concernait pas.
J'ai créé cette issue #675
car je suis pas sur que cette PR soit en lien malgré le titre : #673

Le soucis des tests est en cours d'investigation, c'est un soucis entre lerna et la CI

Ha oui, je pensais que c'était résolu ce souci… En l'état il faudrait attendre la correction de ce problème

Guillaume et Arnaud s'y sont cassé les dents pour le moment...

Qu'est-ce que je fais dans ce cas ? Je pousse le snapshot modifié ou pas ?

Pousse pas le help en effet, j'ai voulu le pousser aussi (j'ai le meme soucis sur ma PR), pousse que ce qui a été directement impacté par ta PR

romuleald · 2020-09-09T14:10:59Z

packages/tabs/src/Tab.tsx

@@ -1,10 +1,5 @@
 import * as React from 'react';


thought : tu n'as rien changé à part l'indent', tu devrais peut-être supprimer cette modif'

romuleald · 2020-09-09T14:18:31Z

packages/tabs/src/TabsCore.tsx


  return (
    <TabsStateless
-      activeIndex={activeIndex || state.activeIndex}
+      activeIndex={state.activeIndex}
      {...otherProps}
      onChange={onChangeEvent(onChange)(setState)(state)}


suggestion : on pourrait peut-être renommer onChangeEvent en setActiveIndex

pas chaud pour faire ce renommage car le onChangeEvent permet de base de modifier le activeIndex mais permet aussi d'exécuter une fonction supplémentaire (paramètre onChange)

Une fonction devrait toujours être nommé en fonction de ce qu'elle fait et pas comment elle est appelé :)
Ce n'est pas le sujet de la PR donc c'est pas grave si on ne le fait pas maintenant

romuleald · 2020-09-09T14:22:25Z

packages/tabs/src/TabsCore.tsx

+  let initialState = defaultState;
+
+  if (activeIndex) {
+    initialState.activeIndex = activeIndex;


question : Pourquoi initialState est un objet ? On n'utilise que la clé dedans activeIndex
La possibilité de mutation est un risque de dysfonctionnement, si on a besoin d'un autre state on pourra toujours à l'avenir faire un autre useState

En fait c'est un risque majeur, initialState étant une référence de l'objet defaultState, celle-ci étant global au module TasbCore.tsx sera commune à tous les appels à ce composant, donc un bug potentiel quand il y a 2 fois le composant dans la page.

j'ai fait en sorte que "activeIndex" soit l'état (ce n'est plus un objet). La prop "activeIndex" est obligatoire avec une valeur par défaut à 0.

romuleald · 2020-09-09T14:32:43Z

packages/tabs/src/TabsCore.tsx

+  let initialState = defaultState;
+
+  if (activeIndex) {
+    initialState.activeIndex = activeIndex;


En fait c'est un risque majeur, initialState étant une référence de l'objet defaultState, celle-ci étant global au module TasbCore.tsx sera commune à tous les appels à ce composant, donc un bug potentiel quand il y a 2 fois le composant dans la page.

guillaume-chervet

beau boulot merci !

sisko59 previously approved these changes Sep 9, 2020

View reviewed changes

romuleald reviewed Sep 9, 2020

View reviewed changes

romuleald suggested changes Sep 9, 2020

View reviewed changes

remija dismissed sisko59’s stale review via 3238993 September 10, 2020 07:12

remija force-pushed the fix/tabs-activateindex-oninit branch from 2397fc2 to 3238993 Compare September 10, 2020 07:12

sisko59 mentioned this pull request Sep 10, 2020

[Bug][Test] Snapshot failed for contribute, on Help.spec when clone the master #675

Closed

remija force-pushed the fix/tabs-activateindex-oninit branch from 3238993 to 60f31c8 Compare September 10, 2020 08:07

romuleald previously approved these changes Sep 10, 2020

View reviewed changes

youf-olivier previously approved these changes Sep 11, 2020

View reviewed changes

samuel-gomez-axa previously approved these changes Sep 11, 2020

View reviewed changes

guillaume-chervet previously approved these changes Sep 11, 2020

View reviewed changes

fix(tabs): init tabs with custom active index other than zero

3246fd0

remija dismissed stale reviews from guillaume-chervet, samuel-gomez-axa, youf-olivier, and romuleald via 3246fd0 September 11, 2020 08:33

remija force-pushed the fix/tabs-activateindex-oninit branch from 60f31c8 to 3246fd0 Compare September 11, 2020 08:33

guillaume-chervet approved these changes Sep 11, 2020

View reviewed changes

guillaume-chervet requested a review from samuel-gomez-axa September 11, 2020 08:58

samuel-gomez-axa approved these changes Sep 11, 2020

View reviewed changes

guillaume-chervet merged commit 04c979a into AxaFrance:master Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tabs): init tabs with custom active index other than zero #677

fix(tabs): init tabs with custom active index other than zero #677

remija commented Sep 9, 2020

sisko59 commented Sep 9, 2020

romuleald Sep 9, 2020

remija Sep 10, 2020

sisko59 Sep 10, 2020 •

edited

Loading

youf-olivier Sep 10, 2020

romuleald Sep 10, 2020

youf-olivier Sep 10, 2020

remija Sep 10, 2020

youf-olivier Sep 10, 2020

remija Sep 10, 2020

romuleald Sep 9, 2020

remija Sep 10, 2020

romuleald Sep 9, 2020

remija Sep 10, 2020

romuleald Sep 10, 2020

romuleald Sep 9, 2020

romuleald Sep 9, 2020

remija Sep 10, 2020

romuleald Sep 9, 2020

guillaume-chervet left a comment

fix(tabs): init tabs with custom active index other than zero #677

fix(tabs): init tabs with custom active index other than zero #677

Conversation

remija commented Sep 9, 2020

Related issue

Reference to the issue

Description of the issue

Person(s) for reviewing proposed changes

Important

sisko59 commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisko59 Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaume-chervet left a comment

Choose a reason for hiding this comment

sisko59 Sep 10, 2020 •

edited

Loading