themesberg / flowbite-react

Official React components built for Flowbite and Tailwind CSS
https://flowbite-react.com
MIT License
1.86k stars 416 forks source link

fix: Revert #1244 to address regression w/ Button and DropdownItem #1298

Closed rnicholus closed 6 months ago

rnicholus commented 6 months ago

In #1244, the return type of Button and DropdownItem was changed to ReactNode. This is not an acceptable return type for functional components. The correct return type is JSX.Element. This change fixes the regression from that PR (introduced in v0.7.3) by reverting that PR and accounting for related changes that followed that PR. Also fixes https://github.com/themesberg/flowbite-react/issues/962.

Perhaps there is a way to fulfill the goals of #1244 and solve this regression, but I wasn't able to find time to address that, and #1244 seems flawed enough that perhaps it just needs to be redesigned.

stackblitz[bot] commented 6 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

vercel[bot] commented 6 months ago

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

Name Status Preview Comments Updated (UTC)
flowbite-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 19, 2024 1:44pm
codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.54%. Comparing base (7461173) to head (9447658). Report is 203 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1298 +/- ## ========================================== - Coverage 99.54% 95.54% -4.01% ========================================== Files 163 218 +55 Lines 6621 9691 +3070 Branches 401 560 +159 ========================================== + Hits 6591 9259 +2668 - Misses 30 432 +402 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rluders commented 6 months ago

@coderabbitai review

coderabbitai[bot] commented 6 months ago

Walkthrough

This update focuses on improving type inference and component refactoring within a React project. It addresses as inference issues in Button and DropdownItem components when used with Link components, by adding temporary @ts-expect-error comments alongside TODO notes for future fixes. Additionally, it introduces a genericForwardRef function to better handle refs with generic components, and refactors Button components to utilize this new approach, enhancing code maintainability and readability.

Changes

Files Changes
.../homepage/...
examples/button/...
examples/dropdown/dropdown.customItem.tsx
Added @ts-expect-error with TODO notes for as inference issues.
src/components/Button/... Refactored Button components, updated types, and utilized genericForwardRef.
src/components/Dropdown/... Fixed a typo, added RefCallback import, and refactored DropdownItem component.
src/helpers/generic-forward-ref.ts Introduced genericForwardRef function.

🐇💻✨
In the land of code, where components intertwine,
A rabbit hopped, fixing types, making them align.
With a flick of its tail, errors began to fade,
Refs and props, in harmony, beautifully made.
🌟🚀🐾
"To the future," it whispered, under the moon's glow,
Where code runs smooth, and development can flow.

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 - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

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 .` - `Generate unit-tests 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 tests 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 tests.` - `@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. - The JSON schema for the configuration file is available [here](https://coderabbit.ai/integrations/coderabbit-overrides.v2.json). - 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/coderabbit-overrides.v2.json` ### CodeRabbit Discord Community Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback.
rnicholus commented 6 months ago

Uh, whatever AI you’ve unleashed on this PR is pretty low-quality. All I’ve done here is ‘git revert’ 1 set of changes and partially revert another.

SutuSebastian commented 6 months ago

Uh, whatever AI you’ve unleashed on this PR is pretty low-quality. All I’ve done here is ‘git revert’ 1 set of changes and partially revert another.

Yep, I know that, but seems like the AI does not

rluders commented 6 months ago

@rnicholus AI is just an tool, we still reviewing as humans. 😁

rnicholus commented 6 months ago

Is there anything else i can do to ensure this change is merged & released?

rluders commented 6 months ago

@rnicholus please, no merge with main... rebase it instead!

rnicholus commented 6 months ago

@rnicholus please, no merge with main... rebase it instead!

this can all be squashed on merge, correct?

rluders commented 6 months ago

@rnicholus yes, but it is preferable to rebase with the main instead of merging it. But, anyway...

I'm wondering if this PR is actually fixing the issue, and not just reverting for some broken state. Isn't better if we could address the problem with a definitive fix? What would be the pros and cons of accepting this to fix #962 and reverting the fix for #1002 and #1107?

rnicholus commented 6 months ago

@rnicholus yes, but it is preferable to rebase with the main instead of merging it. But, anyway...

I'm wondering if this PR is actually fixing the issue, and not just reverting for some broken state. Isn't better if we could address the problem with a definitive fix? What would be the pros and cons of accepting this to fix #962 and reverting the fix for #1002 and #1107?

I'm not sure. My motivation here was to fix the typing issue without having to resort to ts-ignore in my project, and i suspect a bunch of other users are in the same boat.

Isn't better if we could address the problem with a definitive fix?

absolutely, but i'm not sure what that fix is as i don't really have the cycles to investigate

SutuSebastian commented 6 months ago

Closed in favor of #1308