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(popover): trigger disabled prop warning #2784

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

jrgarciadev
Copy link
Member

@jrgarciadev jrgarciadev commented Apr 18, 2024

Closes #

📝 Description

Fix isDisabled prop warning on NextUI components that don't support this propterty, it is also fixed for non-NextUI components.

⛳️ Current behavior (updates)

isDisabled react warning

🚀 New behavior

Warning fixed

💣 Is this a breaking change (Yes/No): No

📝 Additional Information

Summary by CodeRabbit

  • Bug Fixes

    • Resolved warning issues with the isDisabled prop across various components.
    • Enhanced prop handling and element cloning logic in the PopoverTrigger component.
  • New Features

    • Added isDisabled property to the dropdown component for improved accessibility and functionality, including a custom HTML trigger option.
  • Refactor

    • Updated properties and control flows in popover and dropdown components to enhance consistency and performance.

Copy link

changeset-bot bot commented Apr 18, 2024

🦋 Changeset detected

Latest commit: 1a9f6da

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@nextui-org/dropdown Patch
@nextui-org/popover Patch
@nextui-org/theme Patch
@nextui-org/react Patch
@nextui-org/autocomplete Patch
@nextui-org/date-picker Patch
@nextui-org/select Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link
Contributor

coderabbitai bot commented Apr 18, 2024

Walkthrough

This update focuses on refining the handling of the isDisabled property across various components in the NextUI library. Changes include better integration of the isDisabled property in dropdown and popover components, enhancing prop management, and improving the user interface by adjusting control settings in storybook files. These adjustments ensure more consistent behavior and clearer warnings across both NextUI and non-NextUI components.

Changes

File Path Change Summary
.changeset/five-lemons-admire.md Introduced patches for multiple NextUI packages to fix isDisabled prop warnings.
.../dropdown/src/use-dropdown.ts Moved isDisabled property for better parameter organization in useDropdown.
.../dropdown/stories/dropdown.stories.tsx Added isDisabled control and a custom trigger component in dropdown stories.
.../popover/src/popover-trigger.tsx Adjusted prop handling and element cloning in PopoverTrigger; refined isDisabled management.
.../popover/src/use-popover.ts, .../theme/src/components/popover.ts Enhanced isDisabled handling in usePopover and renamed related property in theme settings.

Recent Review Details

Configuration used: .coderabbit.yaml

Commits Files that changed from the base of the PR and between a644544 and 1a9f6da.
Files selected for processing (6)
  • .changeset/five-lemons-admire.md (1 hunks)
  • packages/components/dropdown/src/use-dropdown.ts (1 hunks)
  • packages/components/dropdown/stories/dropdown.stories.tsx (4 hunks)
  • packages/components/popover/src/popover-trigger.tsx (2 hunks)
  • packages/components/popover/src/use-popover.ts (1 hunks)
  • packages/core/theme/src/components/popover.ts (1 hunks)
Additional Context Used
LanguageTool (4)
.changeset/five-lemons-admire.md (4)

Near line 7: Possible spelling mistake found.
Context: ...tch "@nextui-org/theme": patch --- Fix isDisabled prop warning on NextUI components that...


Near line 7: Possible spelling mistake found.
Context: ...h --- Fix isDisabled prop warning on NextUI components that don't support this prop...


Near line 7: Possible spelling mistake found.
Context: ...xtUI components that don't support this propterty, it is also fixed for non-NextUI compon...


Near line 7: Possible spelling mistake found.
Context: ...rt this propterty, it is also fixed for non-NextUI components.

Additional comments not posted (9)
packages/components/popover/src/popover-trigger.tsx (3)

32-32: Ensure that the destructuring of isDisabled from getTriggerProps is correctly utilized in the component logic.

Verification successful

The verification process has confirmed that the isDisabled prop is utilized correctly within the PopoverTrigger component. It is appropriately passed to the useAriaButton function and conditionally merged into restProps, ensuring that the disabled state is managed effectively across the component.

  • The prop is passed to useAriaButton to manage button-specific attributes based on the disabled state.
  • It is also conditionally included in restProps which are passed to the button component, further ensuring that the disabled state influences the button's behavior.
Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that `isDisabled` is used appropriately within the component.
ast-grep --lang javascript --pattern $'const {onPress, isDisabled, ...restProps} = useMemo(() => {
  return getTriggerProps($_, $_);
}, $$$)'

Length of output: 142



Script:

#!/bin/bash
# Search for any usage of `isDisabled` within the PopoverTrigger component
rg "isDisabled" packages/components/popover/src/popover-trigger.tsx

Length of output: 285


39-39: Verify that useAriaButton correctly handles the isDisabled prop.


47-47: Ensure that the merging of props handles the isDisabled state correctly when a NextUI Button is present.

Verification successful

The verification process confirms that the mergeProps function is used correctly in the popover-trigger.tsx file to handle the isDisabled state when a NextUI Button is present. The specific line of code:

mergeProps(restProps, hasNextUIButton ? {onPress, isDisabled} : buttonProps),

shows that when hasNextUIButton is true, both onPress and isDisabled are included in the properties, which aligns with the intended functionality.

  • File: packages/components/popover/src/popover-trigger.tsx
  • Line: 47
Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Ensure correct merging of props in the presence of a NextUI Button.
ast-grep --lang javascript --pattern $'return cloneElement($_, mergeProps(restProps, hasNextUIButton ? {onPress, isDisabled} : buttonProps))'

Length of output: 142



Script:

#!/bin/bash
# Broaden the search to find any usage of `mergeProps` in the file `popover-trigger.tsx`.
rg "mergeProps" packages/components/popover/src/popover-trigger.tsx

Length of output: 271

packages/components/dropdown/src/use-dropdown.ts (1)

50-50: Confirm that the reordering of isDisabled above type in the destructuring does not affect the logic negatively.

packages/core/theme/src/components/popover.ts (1)

165-165: Ensure that the renaming of isDropdownDisabled to isTriggerDisabled is consistently applied across the entire codebase.

Verification successful

The search for the term isDropdownDisabled across JavaScript files using rg did not yield any results. This suggests that the renaming of isDropdownDisabled to isTriggerDisabled has been consistently applied across the codebase, as no instances of the old property name were found.

Conclusion:
The renaming of isDropdownDisabled to isTriggerDisabled appears to have been successfully and consistently applied across the entire codebase. No instances of the old property name were detected in JavaScript files.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for any remaining instances of `isDropdownDisabled` that should be renamed.
rg --type javascript 'isDropdownDisabled'

Length of output: 76



Script:

#!/bin/bash
# Search for any remaining instances of `isDropdownDisabled` that should be renamed, using the correct file type.
rg --type js --type jsx 'isDropdownDisabled'

Length of output: 72



Script:

#!/bin/bash
# Search for any remaining instances of `isDropdownDisabled` that should be renamed, using the correct recognized file type.
rg --type js 'isDropdownDisabled'

Length of output: 33

packages/components/popover/src/use-popover.ts (2)

256-256: Verify that the extraction and usage of isDisabled in getTriggerProps function are correctly implemented.


268-268: Ensure that the isTriggerDisabled property is correctly applied to the trigger element.

packages/components/dropdown/stories/dropdown.stories.tsx (2)

87-91: Ensure that the isDisabled property is correctly documented and implemented in the storybook controls.


578-594: Review the implementation of the CustomHTMLTrigger component to ensure it meets the intended functionality and adheres to accessibility standards.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

vercel bot commented Apr 18, 2024

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

Name Status Preview Comments Updated (UTC)
nextui-docs-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 18, 2024 3:25pm
nextui-storybook-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 18, 2024 3:25pm

@jrgarciadev jrgarciadev marked this pull request as ready for review April 18, 2024 16:21
@jrgarciadev jrgarciadev merged commit 183a4a6 into main Apr 18, 2024
8 checks passed
@jrgarciadev jrgarciadev deleted the fix/popover-trigger-non-react-props branch April 18, 2024 16:22
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

"@nextui-org/theme": patch
---

Fix `isDisabled` prop warning on NextUI components that don't support this propterty, it is also fixed for non-NextUI components.
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct the spelling mistake in "propterty".

- Fix `isDisabled` prop warning on NextUI components that don't support this propterty, it is also fixed for non-NextUI components.
+ Fix `isDisabled` prop warning on NextUI components that don't support this property, it is also fixed for non-NextUI components.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
Fix `isDisabled` prop warning on NextUI components that don't support this propterty, it is also fixed for non-NextUI components.
Fix `isDisabled` prop warning on NextUI components that don't support this property, it is also fixed for non-NextUI components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant