Skip to content

fix(Fieldset): rmv orientation and role and legend is now mandatory#1077

Open
LarryMatte wants to merge 10 commits into
masterfrom
dev/DS-1288
Open

fix(Fieldset): rmv orientation and role and legend is now mandatory#1077
LarryMatte wants to merge 10 commits into
masterfrom
dev/DS-1288

Conversation

@LarryMatte

@LarryMatte LarryMatte commented Feb 3, 2025

Copy link
Copy Markdown
Contributor

https://equisoft.atlassian.net/browse/DS-1288

Enlever l'option d'avoir une orientation horizontal ou vertical sur le fieldset.
La legend n'est plus optional.
Il y avait également un attribut role qui avait été ajouté mais il n'était pas nécessaire.

Pour les tokens de typo, j'ai ajouté ceux manquant mais il va p-e y avoir des modifications à apporter suite à la carte de Marc-andré https://equisoft.atlassian.net/browse/DS-1289

@LarryMatte LarryMatte requested a review from a team as a code owner February 3, 2025 15:52
@LarryMatte LarryMatte changed the title fix(Fieldset): rmv orientation and role fix(Fieldset): rmv orientation and role and legend is now mandatory Feb 3, 2025
@github-actions

github-actions Bot commented Feb 3, 2025

Copy link
Copy Markdown

Storybook for this build: https://ds.equisoft.io/pr-1077/

@github-actions

github-actions Bot commented Feb 3, 2025

Copy link
Copy Markdown

Webapp for this build: https://ds.equisoft.io/pr-1077/webapp/

@pylafleur pylafleur left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ça va clasher un peu avec la PR des tokens de texte... Je crois qu'on devrait merger les tokens en premier, comme ça tu n'auras pas à les ajouter ici.

Comment thread packages/react/src/components/fieldset/fieldset.tsx Outdated

@maboilard maboilard left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

C'est normal que les radio-button sont alignés à l'horizontal?

@LarryMatte

Copy link
Copy Markdown
Contributor Author

C'est normal que les radio-button sont alignés à l'horizontal?

Ouin en s'en fou un peu, le layout devrait être géré à l'extérieur du composant (avec une grid par exemple).

maboilard
maboilard previously approved these changes Feb 13, 2025
@pylafleur

pylafleur commented Feb 13, 2025

Copy link
Copy Markdown
Contributor

@LarryMatte Je viens de merger la PR des tokens de texte. Tu pourras probablement faire un rebase et tous les tokens dont tu as besoin devraient déjà être disponibles s'ils sont conformes au document de Marc.

@pylafleur pylafleur left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

C'est probablement pas dans le scope de cette story mais les checkbox ne sont pas visiblement disabled lorsque leur fieldset l'est. Faudrait probablement utiliser un context dans le fieldset.

Comment thread packages/react/src/components/fieldset/styled.ts Outdated
Comment thread packages/react/src/components/fieldset/fieldset.tsx Outdated
</Legend>
)}
<Legend
{...legendProps /* eslint-disable-line react/jsx-props-no-spreading */}

@pylafleur pylafleur Feb 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

C'était déjà comme ça mais je sais pas si tant qu'à demander toutes les props dans un objet, on devrait pas demander un component Legend directement? Et peut-être aussi accepter juste une string si tout le reste est par défaut?

Ou encore, legendText, legendSize? À quel point on veut rendre la légende stylable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Voir si ce que j'ai maintenant correspond mieux à ce que tu avais en tête.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants