daisyui / react-daisyui

daisyUI components built with React 🌼
http://react.daisyui.com/
MIT License
891 stars 99 forks source link

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

Closed kodjunkie closed 11 months ago

kodjunkie commented 11 months ago

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[bot] commented 11 months ago

Deploy Preview for react-daisyui processing.

Name Link
Latest commit 395163fe1ee2e9441965bab981915fa5c874213c
Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/64b656bc6b1bad00084cc79c
netlify[bot] commented 11 months ago

Deploy Preview for react-daisyui processing.

Name Link
Latest commit 395163fe1ee2e9441965bab981915fa5c874213c
Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/64b656bc6b1bad00084cc79c
netlify[bot] commented 11 months ago

Deploy Preview for react-daisyui ready!

Name Link
Latest commit 71714a169c2602d3bc579c22fd8fd1d80e8ff7ca
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.

kodjunkie commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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.