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.

Sign up for GitHub

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

Jump to bottom

feat(RadioButton, RadioCard, RadioButtonGroup): extract radio input #999

Merged
merged 12 commits into from
Oct 29, 2024
Merged

feat(RadioButton, RadioCard, RadioButtonGroup): extract radio input #999

merged 12 commits into from
Oct 29, 2024

Conversation

LarryMatte
Copy link
Contributor

@LarryMatte LarryMatte commented Sep 28, 2024 •
edited
Loading

DS-1212

extraire le radio input du composant radioButton afin d'atomiser les composants le plus possible.
Remplacer le radioButton dans les composants:

  • RadioCard
  • RadioButtonGroup
  • DataTable avec la feature single select.

Faites signe s'il y a des breaking changes.

@LarryMatte LarryMatte requested a review from a team as a code owner September 28, 2024 15:15
@github-actions GitHub Actions
Copy link

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

@github-actions GitHub Actions
Copy link

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

WilliamsTardif
WilliamsTardif previously approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@WilliamsTardif WilliamsTardif left a comment

Choose a reason for hiding this comment

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

LGTM

maboilard
maboilard reviewed Oct 15, 2024
View reviewed changes
Copy link

@maboilard maboilard left a comment

Choose a reason for hiding this comment

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

C'est normal que le focus s'affiche en même temps sur la Radio Card et le Radio Button?

image

@LarryMatte
Copy link
Contributor Author

LarryMatte commented Oct 15, 2024 •
edited
Loading

C'est normal que le focus s'affiche en même temps sur la Radio Card et le Radio Button?

Oui, ça permet d'indiquer que le radio button est associé à tout le composant et qu'ils ne sont pas deux éléments distincts. Ça peut également être utile pour les gens qui utilisent des magnifiers et qui auraient seulement une partie du visuel dans l'écran.

maboilard
maboilard previously approved these changes Oct 15, 2024
View reviewed changes
@LarryMatte
Copy link
Contributor Author

@savutsang @meriouma @pylafleur Est-ce que l'un de vous pourrait faire une passe de review?
I thank you.

meriouma
meriouma reviewed Oct 28, 2024
View reviewed changes
packages/react/src/components/radio-button-group/radio-button-group.tsx Outdated
@@ -95,6 +43,7 @@ const InnerContent = styled.div<{ $isExpanded: boolean, $transitionStarted: bool
interface RadioButtonProps {
label: string;
value: string;
inputId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense que ça pourrait être juste id

Suggested change
inputId?: string;
id?: string;

packages/react/src/components/radio-card-group/radio-card.tsx Outdated
import * as S from './styled-components';

export interface RadioCardProps {
checked?: boolean;
children?: ReactNode;
className?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Je sais pas si tu avais évalué l'impact de la modification, mais ça empêche de faire un styled(RadioCard). C'est peut-être ce qu'on veut aussi pour éviter de briser le layout? J'ai fait un petit check et je pense que personne faisait de styled(RadioCard).

packages/react/src/components/radio-card-group/styled-components.ts Outdated
@@ -3,47 +3,33 @@ import { ResolvedTheme } from '../../themes/theme';
import { focus } from '../../utils/css-state';

interface InputContainerProps {
disabled?: boolean;
isDisabled?: boolean;
Copy link
Contributor

@meriouma meriouma Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Je pense que j'aurais gardé disabled, quitte à uniformiser les autres props dans un 2e temps et enlever les is. Ce serait plus proche du html. Ça se pourrait qu'on ait à ajouter $ aussi pour que styled-components soit content dans la prochaine version.

@LarryMatte LarryMatte dismissed stale reviews from maboilard and WilliamsTardif via 2ef7988 October 29, 2024 19:49
@LarryMatte LarryMatte changed the title feat(RadioButton): extract radio input feat(RadioButton, RadioCard, RadioButtonGroup): extract radio input Oct 29, 2024
meriouma
meriouma previously approved these changes Oct 29, 2024
View reviewed changes
@LarryMatte LarryMatte merged commit c691a0c into master Oct 29, 2024
22 checks passed
@LarryMatte LarryMatte deleted the dev/DS-1212 branch October 29, 2024 20:54
@meriouma meriouma mentioned this pull request Nov 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meriouma meriouma meriouma approved these changes

@maboilard maboilard maboilard left review comments

@WilliamsTardif WilliamsTardif WilliamsTardif left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LarryMatte @meriouma @WilliamsTardif @maboilard