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

fix(#1686): remove redundant children prop #1717

Merged
merged 4 commits into from
Nov 12, 2023
Merged

fix(#1686): remove redundant children prop #1717

merged 4 commits into from
Nov 12, 2023

Conversation

Gravy59
Copy link
Contributor

@Gravy59 Gravy59 commented Oct 10, 2023

This pull request resolves #1686.

Rationale for this PR

This PR affects the code for RadioGroupItem in both styles by removing the children prop from the component. The children prop is automatically passed in by the use of the spread operator (...props) and is redundant because it is never used in the component.

This PR shouldn't affect tests, representation, etc. and is merely a cosmetic change. There is no urgent need to merge this.

@vercel
Copy link

vercel bot commented Oct 10, 2023

@Gravy59 is attempting to deploy a commit to the shadcn-pro Team on Vercel.

A member of the Team first needs to authorize it.

@shadcn
Copy link
Collaborator

shadcn commented Oct 16, 2023

Interesting. It looks like we're not rendering children at all actually.

I wonder if we need something like this instead:

return (
  <RadioGroupPrimitive.Item
    ref={ref}
    {...props}
  >
    {children || (
      <RadioGroupPrimitive.Indicator className="flex items-center justify-center">
        <CheckIcon className="h-3.5 w-3.5 fill-primary" />
      </RadioGroupPrimitive.Indicator>
    )}
  </RadioGroupPrimitive.Item>
)

@shadcn shadcn added bug Something isn't working component: radio group labels Oct 16, 2023
@shadcn shadcn self-assigned this Oct 30, 2023
@shadcn shadcn added the area: roadmap This looks great. We'll add it to the roadmap, review and merge. label Oct 30, 2023
@shadcn shadcn added the postpone: more info or changes requested maintainers asked a question or needs more info label Nov 12, 2023
Copy link

vercel bot commented Nov 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 12, 2023 9:28am

@shadcn shadcn removed the postpone: more info or changes requested maintainers asked a question or needs more info label Nov 12, 2023
@kodiakhq kodiakhq bot merged commit 51c8c3d into shadcn-ui:main Nov 12, 2023
5 of 6 checks passed
kjxbyz pushed a commit to muse-ui/muse-ui that referenced this pull request Jun 7, 2024
This pull request resolves shadcn-ui#1686.

## Rationale for this PR

This PR affects the code for `RadioGroupItem` in both styles by removing the `children` prop from the component. The children prop is automatically passed in by the use of the spread operator (`...props`) and is redundant because it is never used in the component.

This PR shouldn't affect tests, representation, etc. and is merely a cosmetic change. There is no urgent need to merge this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: roadmap This looks great. We'll add it to the roadmap, review and merge. automerge bug Something isn't working component: radio group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RadioGroupItem: 'children' is defined but never used.
3 participants