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

feat(look and feel): create look and feel packages #227

Closed
wants to merge 2 commits into from

Conversation

PeioDeLaVega
Copy link

No description provided.

@pplancq
Copy link
Contributor

pplancq commented Mar 27, 2024

Je ne comprend pas l'intérêt de cette PR, peut tu donné plus d'explication sur le pourquoi qu'il faudrait sépare les packages ?

@PeioDeLaVega
Copy link
Author

Je ne comprend pas l'intérêt de cette PR, peut tu donné plus d'explication sur le pourquoi qu'il faudrait sépare les packages ?

Il a été acté en Instance de Design System le 20/02 que le socle technique des Design Systems serait comme suit :

  • 1 repo unique aux 2 DS pour faciliter la convergence lorsque toutes les verticales auront convergé
  • 2 implémentations distinctes : séparation par dossier dans les sources du repo (1 Slash / 1 L&F)
  • 1 référentiel Design unique dédiée aux DS : Zeroheight (alimenté via Figma)

@pplancq
Copy link
Contributor

pplancq commented Mar 28, 2024

Je ne comprend pas l'intérêt de cette PR, peut tu donné plus d'explication sur le pourquoi qu'il faudrait sépare les packages ?

Il a été acté en Instance de Design System le 20/02 que le socle technique des Design Systems serait comme suit :

  • 1 repo unique aux 2 DS pour faciliter la convergence lorsque toutes les verticales auront convergé
  • 2 implémentations distinctes : séparation par dossier dans les sources du repo (1 Slash / 1 L&F)
  • 1 référentiel Design unique dédiée aux DS : Zeroheight (alimenté via Figma)

Et bien justement c'est ce que l'on fait ici sur ce repo, un repo unique pour les 2 DS.
@JLou , @samuel-gomez vous confirmez ?

@samuel-gomez
Copy link

@pplancq je comprends ton questionnement, il y a eu pas mal de débats, je n'étais pas en phase avec cela mais c'est effectivement ce qui a été acté sous condition que la PR soit faite par ceux qui souhaitaient mettre cela en place.
La gestion des versions ne sera pas non plus la même (Slash en SemVer et L&F en CalVer) même si je pense que notre structure actuelle pourrait également répondre à ce besoin.

@samuel-gomez
Copy link

samuel-gomez commented Mar 28, 2024

par contre @PeioDeLaVega des développements ont déjà été effectués sur la partie L&F mais je ne vois pas ces composants basculés dans le nouveau dossier, il était convenu de ne pas perdre le travail accompli 😉

@PeioDeLaVega
Copy link
Author

@samuel-gomez Merci pour ton retour. Effectivement, je n'ai pas encore migré le travail déjà accompli, je préfère y aller par itération. Si le contenu de ma PR vous satisfait, je lancerai le processus de migration du travail déjà accompli dans un second temps sous le contrôle de la gouvernance Look&Feel.

@pplancq
Copy link
Contributor

pplancq commented Mar 29, 2024

@pplancq je comprends ton questionnement, il y a eu pas mal de débats, je n'étais pas en phase avec cela mais c'est effectivement ce qui a été acté sous condition que la PR soit faite par ceux qui souhaitaient mettre cela en place. La gestion des versions ne sera pas non plus la même (Slash en SemVer et L&F en CalVer) même si je pense que notre structure actuelle pourrait également répondre à ce besoin.

Oki je comprend.

Copy link
Contributor

@pplancq pplancq left a comment

Choose a reason for hiding this comment

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

Je pense qu'il aurait mieux valu déplacer au lieu de créer.

De plus le workflow de publish principale dans pas été update et donc par conséquence il va publish tous les packages sans distinction

package.json Outdated
Comment on lines 31 to 32
"packages/look-and-feel/css",
"packages/look-and-feel/react",
Copy link
Contributor

Choose a reason for hiding this comment

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

pourquoi créer un sous-sous package ?

N'aurait-il pas été plus simple de renommer les package css et react en css-agent et react-agent et de créer deux nouveaux package css-client et react-client.

De plus avec ce nommage il nous serait possible de créer des packages css-base et/ou react-base pour des asset commun au 2 design-system

Copy link

Choose a reason for hiding this comment

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

Hello,

Merci pour ton review ! Peut-être que j'ai mal compris mais si la structure que tu proposes est la suivante :

/
|-- packages/
    |-- css-agent/
    |-- css-client/
    |-- react-agent/
    |-- react-client/

alors oui cela aurait été plus simple de renommer les packages mais on ne matérialiserait pas la séparation entre Slash et Look and Feel avec cette structuration applanie.

De plus -agent et -client ne reflètent pas explicitement slash et look-and-feel et cela pourrait induire en erreur ceux qui arrivent sur le repo et qui ne connaissent pas nos design systems, qui pourraient penser que nous n'en avons qu'un, décliné en populations cibles, ce qui n'est pas le cas : nous en avons bien 2 (au moins). Pour moi cette notion de "population cible" (agent/client) pourrait être traduite dans le readme de chaque design system si nécessaire.

Je suis donc d'avis à garder 2 répertoires distincts et clairement nommés du nom de chaque DS, conformément à ce qui a été décidé.

Par contre @PeioDeLaVega il n'est pas nécessaire ici de nommer chaque sous répertoire, tu peux faire

+    "packages/look-and-feel/*"

Copy link
Contributor

Choose a reason for hiding this comment

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

Pour la structure oui c'est bien ça dont je parlait

/
|-- packages/
    |-- css-agent/
    |-- css-client/
    |-- react-agent/
    |-- react-client/

Personnellement je trouve cette structure plus simples et la séparation est bien visible mais ce n'est que mon avis 😉

Après pour le nommage depuis le début on est parti sur agent pour désigner slash et client pour look-and-feel

Cela risque de faire beaucoup de changement pour les contributeurs.

à la limite ce que l'on peux faire c'est

/
|-- packages/
    |-- look-and-feel-css/
    |-- look-and-feel-react/
    |-- slash-css/
    |-- slash-react/

on reste sur une structure à plat des dossiers mais comme le nom du design-system et en préfix cela permet une meilleur visibilité.

Copy link

@jmraxa jmraxa Apr 9, 2024

Choose a reason for hiding this comment

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

Après pour le nommage depuis le début on est parti sur agent pour désigner slash et client pour look-and-feel

Cela risque de faire beaucoup de changement pour les contributeurs.

Yes ça fait beaucoup de changements j'en suis consient, en même temps je trouve normal de faire des ajustements en début de projet. Le risque serait de lier l'avenir du projet à des positions initiales qu'on se refuse de revoir.

En l'occurrence "agent" ou "client" n'ont pas plus de sens que "B2B" ou "B2C" par exemple, le fait est qu'ils s'appellent "Slash" et "Look and Feel" : les obfusquer par "agent", "client" (ou même "B2B" et "B2C" pour reprendre mon exemple) ne servirait qu'à induire la personne qui n'a pas le contexte à penser que nous n'avons qu'un design system !

à la limite ce que l'on peux faire c'est

/
|-- packages/
|-- look-and-feel-css/
|-- look-and-feel-react/
|-- slash-css/
|-- slash-react/

on reste sur une structure à plat des dossiers mais comme le nom du design-system et en préfix cela permet une meilleur visibilité.

Cela faisait sens de grouper les packages css et react au sein d'un répertoire look-and-feel dès lors que ces packages ont un workflow et cycle de vie qu'ils ne partagent pas avec les packages de Slash. Mais on peut adopter cette structure si tu y tiens.

Copy link
Contributor

Choose a reason for hiding this comment

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

C'est juste une question de préférence après on peut remonter d'une arbo package n'as pas vraiment de sens on pour pourrais faire comme ceci

/
|-- look-and-feel/
    |-- css/
    |-- react/
|-- slash/
    |-- css/
    |-- react/

comme cela on a la séparation franche, sa coller avec le sample

après je suis d'accord avec le reste.

Copy link

Choose a reason for hiding this comment

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

/
|-- look-and-feel/
    |-- css/
    |-- react/
|-- slash/
    |-- css/
    |-- react/

Plus simple et le plus clair effectivement, go pour moi.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PeioDeLaVega Je vois que tu as ramener sur

/
|-- packages/
    |-- look-and-feel-css/
    |-- look-and-feel-react/

Moi cela me va, comme dit plus haut j'aime pas trop qu'il y en trop d'arborescence pour les différents packages.

Par contre tu n'a fait que de l'ajout sans aucune modification des

/
|-- packages/
    |-- css/
    |-- react/

est une volonté pour permettre des test les workflow ou ne vaudrait il pas mieux tous séparer d'un coup ?

packages/look-and-feel/config/eslint.config.js Outdated Show resolved Hide resolved
packages/look-and-feel/css/.eslintrc.js Outdated Show resolved Hide resolved
packages/look-and-feel/css/.prettierignore Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

il faudrait changer les steps:

- run: npm version "${{ steps.gitversion.outputs.fullSemVer }}" --workspaces --workspaces-update=true
- run: npm publish --workspace "packages" --access public

par

- run: npm version "${{ steps.gitversion.outputs.fullSemVer }}" -w packages/css -w packages/react
- run: npm publish -w packages/css -w packages/react --access public

Comment on lines 35 to 36
- run: npm version ${{ steps.determine_version.outputs.version }} -w packages/look-and-feel/css -w packages/look-and-feel/react
- run: npm publish -w packages/look-and-feel/css -w packages/look-and-feel/react --access public
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- run: npm version ${{ steps.determine_version.outputs.version }} -w packages/look-and-feel/css -w packages/look-and-feel/react
- run: npm publish -w packages/look-and-feel/css -w packages/look-and-feel/react --access public
- run: npm version ${{ steps.determine_version.outputs.version }} -w packages/look-and-feel-css -w packages/look-and-feel-react
- run: npm publish -w packages/look-and-feel-css -w packages/look-and-feel-react --access public

echo '::set-output name=version::$VERSION'
- run: npm run package:check
- run: npm ci
- run: npm run build
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- run: npm run build
- run: npm run build:look-and-feel

Copy link
Contributor

@johnmeunier johnmeunier left a comment

Choose a reason for hiding this comment

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

Pour la prochaine PR, n'hésite pas à fournir une description détaillé surtout pour ce genre de cas

@jmraxa
Copy link

jmraxa commented Sep 12, 2024

Découpé en #466 et #438

@jmraxa jmraxa closed this Sep 12, 2024
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.

5 participants