Skip to content

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

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

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