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

refactor(DropdownItem): make wrapping of children with anchor tag optional #386

Merged
merged 2 commits into from Jul 21, 2023

Conversation

kodjunkie
Copy link
Contributor

I believe this solves #144 and also, allows the usage of the new updates introduced to daisyUI v3 related to Menu (eg: Menu with active item). The default behavior is the case where the children is wrapped in an anchor tag so it doesn't cause a breaking change.

@netlify
Copy link

netlify bot commented Jul 18, 2023

👷 Deploy Preview for react-daisyui processing.

Name Link
🔨 Latest commit 395163f
🔍 Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/64b656bc6b1bad00084cc79c

1 similar comment
@netlify
Copy link

netlify bot commented Jul 18, 2023

👷 Deploy Preview for react-daisyui processing.

Name Link
🔨 Latest commit 395163f
🔍 Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/64b656bc6b1bad00084cc79c

@netlify
Copy link

netlify bot commented Jul 18, 2023

Deploy Preview for react-daisyui ready!

Name Link
🔨 Latest commit 71714a1
🔍 Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/64b95281befc1b000861bda1
😎 Deploy Preview https://deploy-preview-386--react-daisyui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@benjitrosch benjitrosch left a comment

Choose a reason for hiding this comment

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

@kodjunkie I like the idea, thanks for the PR! My one concern is that then the props don't match (the props are currently typed as React.AnchorHTMLAttributes<HTMLAnchorElement>) and cannot be passed thru to any effect. If you can solve this issue, I'm happy to approve!

@kodjunkie
Copy link
Contributor Author

Yes, that's the case for users who want to use the default wrapped anchor tag. If you're passing a full react node as children and the anchor tag is unnecessary then, the only required prop is anchor={false}.

@benjitrosch
Copy link
Collaborator

Yes, that's the case for users who want to use the default wrapped anchor tag. If you're passing a full react node as children and the anchor tag is unnecessary then, the only required prop is anchor={false}.

Right but the component's props are still typed as anchor attributes and it will still accept props as if that were the underlying element. Because of this a developer could pass in props but nothing will happen, which would be a misleading developer experience.

You could use something like a Discriminated Union which will only type it as AnchorHTMLAttributes if anchor={false} and otherwise just pass through children and nothing else.

@kodjunkie
Copy link
Contributor Author

Yes, that's the case for users who want to use the default wrapped anchor tag. If you're passing a full react node as children and the anchor tag is unnecessary then, the only required prop is anchor={false}.

Right but the component's props are still typed as anchor attributes and it will still accept props as if that were the underlying element. Because of this a developer could pass in props but nothing will happen, which would be a misleading developer experience.

You could use something like a Discriminated Union which will only type it as AnchorHTMLAttributes if anchor={false} and otherwise just pass through children and nothing else.

You're right, I just pushed a fix.

Copy link
Collaborator

@benjitrosch benjitrosch left a comment

Choose a reason for hiding this comment

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

@kodjunkie thanks for making the change, looks good!

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

2 participants