Feature/improve contributor xp #729

gcruchon · 2020-10-18T19:36:50Z

Related issue

Reference to the issue

Fix #693

Description of the issue

Improving contributor experience by adding:

List of supported browsers
description of a component
description of tools
link to MS Eadge VM
list of scripts and what they do

This is a first version, do not hesitate to comment and request for more docs.

Person(s) for reviewing proposed changes

@arnaudforaison @samuel-gomez @guillaumechervet @xballoy @youf-olivier

CONTRIBUTING.md

xballoy · 2020-10-19T07:01:53Z

Sync with @samuel-gomez because he did some modifications regarding the design system on #728.
Maybe you should move those modifications in the new repo.

arnaudforaison · 2020-10-20T14:54:23Z

@gcruchon les modifications ont été faites, est ce que tu pourrais les mettre au bon endroit ou les revoir si nécessaire.

gcruchon · 2020-10-24T08:46:55Z

OK, @xballoy and @arnaudforaison, I am going to rebase, but I hope you see that 2 separate repos are not really optimized for a simple contribution experience ;-)

gcruchon · 2020-10-24T14:22:45Z

Waiting for comments from @youf-olivier (he has some ideas to add, as I understood) to finalize the PR.

youf-olivier · 2020-10-24T16:22:57Z

Alors il faut ajouter :

Ca tourne que sur Node 12+
En cas de migration ou de problème il existe une commande (COMMAND OF THE DEATH car très gourmande) qui clean les repo (non lerna clean marche pas) : "find . -name 'node_modules' -type d -prune -exec rm -rf '{}' +" => Ca va dans tous les repo clean le node_module. J4ai du le faire quand j'ai switch de node 10 a 12.

gcruchon · 2020-10-24T20:55:35Z

Merci @youf-olivier !

Pour le "ça tourne que sur Node 12+", j'ai gardé ce que @xballoy avait mis dans sa PR :

You must use a version of Node.js respecting the following rule >=10.14.2 or 12.x or >=14.

Pour les soucis de migration, en phase aussi, j'utilise beaucoup find . -name "node_modules" -type d -prune -exec rm -rf '{}' +
Et pour aller plus vite d'un npm run clean, j'utilise son frère find . -name "node_modules" -type d -prune -exec rm -rf '{}' +

Mais c'est mal, parce que :

C'est propre à Mac / Linux et ne marchera pas sur un PC...
En phase, lerna clean devrait faire la même chose que find . -name "node_modules" -type d -prune -exec rm -rf '{}' +, en étant agnostique à l'OS, il ne supprimera pas le node_modules de la racine, mais ça me va très bien. Il faudra juste faire un npm ci à la racine pour s'assurer qu'on repart de zéro. As-tu un cas précis pour lequel ça ne marche pas?

Du coup, je viens de poster un update qui :

ajoute un script clean:node_modules qui permet de faire tourner lerna clean
ajoute un script clean:dist qui reprend l'ancien fonctionnement du script clean
modifie le script clean pour faire tourner les deux sous-scripts
modifie la doc en conséquence (et ajoute la commande de la mort :))

CONTRIBUTING.md

xballoy · 2020-10-25T14:10:17Z

Yes, it's fine on master

…

-------- Original Message --------

On 25 Oct 2020, 14:07, Gilles CRUCHON wrote: @gcruchon commented on this pull request. --------------------------------------------------------------- In [CONTRIBUTING.md](#729 (comment)): > + +### Documentation + +After working on a component, you are expected to update the documentation. + +Documentation can be found in the [Design System](https://github.com/AxaGuilDEv/design-system) repository. + +Do not forget to update the component status page, located in this repository at: + +```text +└─ src/ + └─ pages/ + └─ get-started/ + └─ component-status/ + └─ markdown/ + └─ content.md True, that makes sense now that it's in a different repo (though there is a link on line 145) NB: link will be on master, is that OK? — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#729 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAFHRYLSMIUGPZCASVIFAWDSMQPK7ANCNFSM4SVJKMXQ).

gcruchon · 2020-10-25T14:14:24Z

Then all should OK for a merge ▶️

😅

arnaudforaison · 2020-10-26T09:48:24Z

CONTRIBUTING.md

+For this, you will add a remote which points to the original repository (this has to be done only once):
+
+```sh
+git remote add upstream https://github.com/AxaGuilDEv/react-toolkit


Ce n'est pas plutôt https://github.com/AxaGuilDEv/react-toolkit.git

Will fix, thanks

arnaudforaison · 2020-10-26T09:49:27Z

CONTRIBUTING.md

+```sh
+git checkout master
+git fetch upstream
+git rebase upstream/master


La doc dit de faire un merge, utile de le préciser ici ?

Both merge and rebase are working, what's your issue with rebase?

https://medium.com/@topspinj/how-to-git-rebase-into-a-forked-repo-c9f05e821c8a

Les 2 fonctionnent mais la doc officielle GitHub dit de faire un merge. Peut importe c'est bien comme ça.

This only thing: if, by mistake you started to work on your master branch, rebase will be easier to understand in the history.

Other issue I faced today: I worked on 2 different branches, my first branch (this one) is taking more time to be reviewed than my second branch (brandUlr). My second branch was merged into upstream repo before my first branch. Without rebase, it was a total mess in the history :)

I also prefer to rebase, I would keep it like that

arnaudforaison · 2020-10-26T09:50:19Z

CONTRIBUTING.md

+├── src/
+    ├── __snapshots__ : generated storybook snapshots used to test the component.
+    ├── GreatComponent.js : the react component code.
+    ├── GreatComponent.md : more documentation about the component (props, ...).


Sauf erreur de ma part mais il n' y a que des README.md

Well... 9 occurrences... of such.
So what is what's the proper way to do? :)

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Form/Input/date/src/CustomDate.md

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Form/Input/date/src/InputDate.md

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Form/Input/choice/src/Choice.md

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Form/Input/switch/src/Switch.md

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Form/Input/date-phone/src/DatePhone.md

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Form/Input/date-phone/src/InputDate.md

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Layout/header/src/NavBar/NavBarItem.md

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Layout/header/src/NavBar/NavBar.md

https://github.com/AxaGuilDEv/react-toolkit/blob/master/packages/Layout/header/src/ToggleButton/ToggleButton.md

arnaudforaison · 2020-10-26T09:52:08Z

CONTRIBUTING.md

+Do not forget to update the component status page, located in this repository at:
+
+```text
+└─ src/


Est ce que c'est vraiment l'endroit pour préciser cela ?

I'd root for that, even more since the code was split in another repo :) Not everybody have the history.

Le code n'est pas splitté en plusieurs repo. On a d'un côté un site de présentation des guidelines et de l'autre (ici) l'implémentation des ces guidelines en React. 2 choses totalement différente

Those 2 things were in the same repo before, that's why I call it a split :-D

Anyway, the end result: if you create / update a component here, you have to update the guideline as well, and it's a job to do in another repo. Therefore, if you are a newbie, like me, you are happy to know it.

I think you are right, I should not indicate the path to this "component-status" page (it may change one day). Is it OK to keep only a reminder (I just pushed that)?

arnaudforaison · 2020-10-26T09:53:27Z

CONTRIBUTING.md

+
+Keep in mind, though, that your pull request will be squashed into master, so repository mainteners may use your commit message but are not entitled to use them as is.
+
+## Scripts


Pour moi c'est de trop car les scripts évoluent et si le dev veut les voir il suffit d'aller sur le package.json

Not everybody is a jedi master. Think of this as a way to help newcomers.

I agree with Gilles, we never have too much documentation

arnaudforaison · 2020-10-26T09:55:44Z

package.json

@@ -9,13 +9,13 @@
  "scripts": {
    "publish": "lerna publish",
    "postinstall": "npm run clean && lerna bootstrap --no-ci && npm run style",
-    "clean": "node ./scripts/clean.js",
+    "clean": "npm run clean:node_modules && npm run clean:dist",
+    "clean:dist": "node ./scripts/clean.js",


Peut etre utiliser un lerna exec couplé avec rimraf (le plus performant sur windows et utiliser par lerna clean)

Not sure I get it...

Rimraf being used in both lerna clean and clean.js what would you like?

I am not sure we should rewrite lerna clean. If any bug, let's fix it in Lerna's repo.

As for clean.js, if any bug / improvement, let's fix it in another PR?

gcruchon · 2020-10-26T12:06:40Z

Je vous laisse merger, du coup? :)

arnaudforaison · 2020-10-27T17:27:11Z

Je vous laisse merger, du coup? :)

Je t'ai fait des retours @gcruchon

…component code

…ystem repo

gcruchon · 2020-10-31T09:35:50Z

Rebased & Pushed some fixes.

gcruchon · 2020-11-05T19:59:04Z

One more reviewer? @youf-olivier @arnaudforaison, are you OK with those fixes?

gcruchon · 2020-11-06T22:48:00Z

Should I rebase it or you can actually squash it in master?

youf-olivier · 2020-11-07T08:58:03Z

Clairement je suis pas un super fan du nouveau bouton :

Ca va faire des historique dégueu avec de merge de la master vers la branche (cf le post #786)

xballoy reviewed Oct 19, 2020

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

xballoy previously approved these changes Oct 19, 2020

View reviewed changes

xballoy added the hacktoberfest-accepted label Oct 19, 2020

gcruchon dismissed xballoy’s stale review via 4f89249 October 24, 2020 13:53

gcruchon force-pushed the feature/improve-contributor-xp branch from 154d211 to 4f89249 Compare October 24, 2020 13:53

xballoy previously approved these changes Oct 25, 2020

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

gcruchon dismissed xballoy’s stale review via d7e12c5 October 25, 2020 13:07

xballoy previously approved these changes Oct 25, 2020

View reviewed changes

youf-olivier previously approved these changes Oct 26, 2020

View reviewed changes

arnaudforaison reviewed Oct 26, 2020

View reviewed changes

gcruchon dismissed stale reviews from youf-olivier and xballoy via de37273 October 31, 2020 09:20

gcruchon force-pushed the feature/improve-contributor-xp branch from 1f172ae to de37273 Compare October 31, 2020 09:20

gcruchon added 5 commits October 31, 2020 10:21

docs(contributing): improve contributing experience

9bbaff4

chore: remove useless "design" npm scripts

1f3f8e1

chore(clean): improve the cleaning process

52d546a

feat(contributing): take into account the fact that stories are near …

4e5e466

…component code

docs(contributing): fix some URLs

1585b92

gcruchon force-pushed the feature/improve-contributor-xp branch from de37273 to 1585b92 Compare October 31, 2020 09:22

docs(contributing): removing path to component status on the design s…

ed50730

…ystem repo

xballoy approved these changes Nov 2, 2020

View reviewed changes

arnaudforaison approved these changes Nov 5, 2020

View reviewed changes

Merge branch 'master' into feature/improve-contributor-xp

ed138e9

Merge branch 'master' into feature/improve-contributor-xp

92e840b

youf-olivier merged commit 220fdb0 into AxaFrance:master Nov 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/improve contributor xp #729

Feature/improve contributor xp #729

gcruchon commented Oct 18, 2020 •

edited by arnaudforaison

Loading

xballoy commented Oct 19, 2020

arnaudforaison commented Oct 20, 2020

gcruchon commented Oct 24, 2020

gcruchon commented Oct 24, 2020

youf-olivier commented Oct 24, 2020

gcruchon commented Oct 24, 2020

xballoy commented Oct 25, 2020 via email

gcruchon commented Oct 25, 2020

arnaudforaison Oct 26, 2020

gcruchon Oct 31, 2020

arnaudforaison Oct 26, 2020

gcruchon Oct 31, 2020

arnaudforaison Oct 31, 2020

gcruchon Oct 31, 2020

xballoy Nov 2, 2020

arnaudforaison Oct 26, 2020

gcruchon Oct 31, 2020

arnaudforaison Oct 26, 2020

gcruchon Oct 31, 2020

arnaudforaison Oct 31, 2020

gcruchon Oct 31, 2020 •

edited

Loading

arnaudforaison Oct 26, 2020

gcruchon Oct 31, 2020

xballoy Nov 2, 2020

arnaudforaison Oct 26, 2020

gcruchon Oct 31, 2020

gcruchon commented Oct 26, 2020

arnaudforaison commented Oct 27, 2020

gcruchon commented Oct 31, 2020

gcruchon commented Nov 5, 2020

gcruchon commented Nov 6, 2020

youf-olivier commented Nov 7, 2020


		Keep in mind, though, that your pull request will be squashed into master, so repository mainteners may use your commit message but are not entitled to use them as is.

		## Scripts

Feature/improve contributor xp #729

Feature/improve contributor xp #729

Conversation

gcruchon commented Oct 18, 2020 • edited by arnaudforaison Loading

Related issue

Reference to the issue

Description of the issue

Person(s) for reviewing proposed changes

xballoy commented Oct 19, 2020

arnaudforaison commented Oct 20, 2020

gcruchon commented Oct 24, 2020

gcruchon commented Oct 24, 2020

youf-olivier commented Oct 24, 2020

gcruchon commented Oct 24, 2020

xballoy commented Oct 25, 2020 via email

gcruchon commented Oct 25, 2020

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

gcruchon Oct 31, 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

gcruchon commented Oct 26, 2020

arnaudforaison commented Oct 27, 2020

gcruchon commented Oct 31, 2020

gcruchon commented Nov 5, 2020

gcruchon commented Nov 6, 2020

youf-olivier commented Nov 7, 2020

gcruchon commented Oct 18, 2020 •

edited by arnaudforaison

Loading

gcruchon Oct 31, 2020 •

edited

Loading