Skip to content

fix(ui-library): radio component fixes#1237

Open
ashk1996 wants to merge 3 commits intodevelopfrom
fix/764_radio-component-fixes
Open

fix(ui-library): radio component fixes#1237
ashk1996 wants to merge 3 commits intodevelopfrom
fix/764_radio-component-fixes

Conversation

@ashk1996
Copy link
Contributor

@ashk1996 ashk1996 commented Mar 27, 2025

closes #916 closes #479 closes #824 closes #764 closes #1073 closes #1141 closes #766

@ashk1996 ashk1996 requested a review from thrbnhrtmnn March 27, 2025 14:14
@thrbnhrtmnn
Copy link
Contributor

Hey @ashk1996 ,
here are my findings:

Radio - remove error message #916

  • When I set hasError=true, an error message is shown. This should not be there.

Radio - add missing events #479

  • In the ticket an onChange event should have been added instead of the onClick event.

Radio - fix value prop #824

  • no remarks here, all looks good

Radio - fix size of input control and icon and add optional max width #764

  • I think this one we should have a quick look together at. I am not sure how I can test this, as the tokens have the same value in each size-variant.

I will continue testing this one later. I only got to check half of the tickets done by this PR.

@thrbnhrtmnn
Copy link
Contributor

Hey @ashk1996 ,

I finished looking at the remaining tickets. All looks fixed here, except I didn't know what exactly needed to be changed with #1072 for the radio component. Therefor I can not review the ACs of this ticket. I would leave this one up to @ChristianHoffmannS2 .

Unfortunately I did not see any changes to the previous comments. Please have a look again at these and let me know once I can review them again. Or maybe did you just forget to push the last changes?

@thrbnhrtmnn
Copy link
Contributor

Hey @ashk1996 ,
as just reviewed together there is just one open point:

Radio - remove error message #916

  • You removed the error message, but overlooked one spacing, which comes from the form-caption-group the error message was part of:
    Bildschirmfoto 2025-04-30 um 14 47 07
    Bildschirmfoto 2025-04-30 um 14 47 17
  • The form caption group only should be part of the component, if we also show a caption. In this case for the radio this should only happen when hasHint is true.

Copy link
Contributor

@thrbnhrtmnn thrbnhrtmnn left a comment

Choose a reason for hiding this comment

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

Hey @ChristianHoffmannS2 , I found one issue. When hasError=true, and checked=true, the control is displayed in the wrong color:
radio-issue
This also happens in dark mode:
radio-dark-issue

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

Labels

None yet

Projects

None yet

3 participants