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: Radio, CheckboxのProps型を修正 #1560

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

Conversation

Qs-F
Copy link
Collaborator

@Qs-F Qs-F commented Apr 8, 2024

User description

チケット

実装内容

  • Radio, CheckboxのProps型をrefが正しく HTMLInputElement に推論されるように修正

スクリーンショット

変更前 変更後

相談内容(あれば)


Type

bug_fix, enhancement


Description

  • Refactored Checkbox and Radio components to correctly infer the ref prop type as HTMLInputElement.
  • Introduced internal prop types _CheckboxProps and _RadioProps for Checkbox and Radio components respectively.
  • Exported CheckboxProps and RadioProps using ComponentPropsWithRef to ensure correct ref typing.

Changes walkthrough

Relevant files
Enhancement
Checkbox.tsx
Refactor Checkbox Props for Correct Ref Typing                     

packages/for-ui/src/checkbox/Checkbox.tsx

  • Imported ComponentPropsWithRef from react.
  • Changed the CheckboxProps type to use ComponentPropsWithRef.
  • Updated the Checkbox component to use a new internal props type
    _CheckboxProps.
  • Exported CheckboxProps as ComponentPropsWithRef of the Checkbox
    component.
  • +5/-3     
    Radio.tsx
    Refactor Radio Props for Correct Ref Typing                           

    packages/for-ui/src/radio/Radio.tsx

  • Imported ComponentPropsWithRef from react.
  • Changed the RadioProps type to use ComponentPropsWithRef.
  • Updated the Radio component to use a new internal props type
    _RadioProps.
  • Exported RadioProps as ComponentPropsWithRef of the Radio component.
  • +5/-3     

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    changeset-bot bot commented Apr 8, 2024

    ⚠️ No Changeset found

    Latest commit: c81e433

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Description updated to latest commit (4bca22b)

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and mainly involve refactoring the props types for better type inference. The modifications are limited to type definitions and the way props are passed to the forwardRef function, which are relatively simple to review for someone familiar with TypeScript and React.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Missing Tests: The PR does not mention adding or updating tests to cover the changes in prop types. It's important to ensure that these changes do not introduce regressions.

    Type Compatibility: While the changes aim to improve type inference, there's no explicit mention of testing for backward compatibility with existing usages of Checkbox and Radio components. It would be beneficial to ensure that these changes do not break existing implementations.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    cloudflare-pages bot commented Apr 8, 2024

    Deploying for-ui with  Cloudflare Pages  Cloudflare Pages

    Latest commit: c81e433
    Status: ✅  Deploy successful!
    Preview URL: https://86fcafb4.for-ui.pages.dev
    Branch Preview URL: https://fix-radio-checkbox-ref-type.for-ui.pages.dev

    View logs

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Rename _CheckboxProps to avoid confusion with private naming conventions.

    Consider renaming _CheckboxProps to CheckboxPropsInternal or another name that does not
    start with an underscore. In TypeScript, names starting with an underscore are often used
    to denote private properties or methods and might be misleading when used for type
    definitions that are meant to be internal but still exported.

    packages/for-ui/src/checkbox/Checkbox.tsx [8]

    -type _CheckboxProps = MuiCheckboxProps & {
    +type CheckboxPropsInternal = MuiCheckboxProps & {
     
    Ensure ref prop is properly forwarded in Checkbox.

    Since ComponentPropsWithRef is used to define CheckboxProps, it's
    important to ensure that the props passed to Checkbox are properly forwarded to the
    underlying MuiCheckbox component, especially the ref. This ensures that the ref prop works
    as expected for consumers of Checkbox.

    packages/for-ui/src/checkbox/Checkbox.tsx [35]

    -export const Checkbox = forwardRef<HTMLInputElement, _CheckboxProps>(
    +export const Checkbox = forwardRef<HTMLInputElement, CheckboxPropsInternal>(
     
    Rename _RadioProps to avoid confusion with private naming conventions.

    Similar to the Checkbox component, consider renaming _RadioProps to RadioPropsInternal or
    another name that avoids starting with an underscore for consistency and clarity.

    packages/for-ui/src/radio/Radio.tsx [7]

    -type _RadioProps = Omit<MuiRadioProps, 'ref'> & {
    +type RadioPropsInternal = Omit<MuiRadioProps, 'ref'> & {
     
    Ensure ref prop is properly forwarded in Radio.

    Ensure that all props, including ref, are correctly forwarded to the MuiRadio component
    within the Radio component to maintain consistency and functionality.

    packages/for-ui/src/radio/Radio.tsx [23]

    -export const Radio = forwardRef<HTMLInputElement, _RadioProps>(
    +export const Radio = forwardRef<HTMLInputElement, RadioPropsInternal>(
     
    Maintainability
    Explicitly define CheckboxProps for clarity and maintainability.

    For better maintainability and to avoid potential future conflicts, consider explicitly
    defining the props that Checkbox accepts instead of using ComponentPropsWithRef<typeof
    Checkbox>. This approach provides clearer documentation and type checking for consumers of
    the Checkbox component.

    packages/for-ui/src/checkbox/Checkbox.tsx [62]

    -export type CheckboxProps = ComponentPropsWithRef<typeof Checkbox>
    +export type CheckboxProps = CheckboxPropsInternal & { ref?: Ref<HTMLInputElement> }
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @Qs-F Qs-F requested a review from locona April 8, 2024 12:38
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Radio, Checkboxのrefの型がHTMLButtonElementとして推論されてしまう
    1 participant