daisyui / react-daisyui

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

fix: Removed generic on Select #276

Closed sahinvardar closed 1 year ago

sahinvardar commented 1 year ago

In favor to be compliant with the standard API I removed the generic support on Select. I got your idea why you introduced that but the problem with the current implementation is that your custom onChange also does not really work.

event.target.value is always a string, the type does not change just by casting it to T.

Also for Select ,all of the sudden the standard way of using the select component does not work anymore and I think this could lead to problems and confusion.

I also assume that this would solve

https://github.com/daisyui/react-daisyui/issues/264 https://github.com/daisyui/react-daisyui/issues/272

Which needs to be confirmed.

Let me know what you think

Best Sahin

netlify[bot] commented 1 year ago

Deploy Preview for react-daisyui processing.

Name Link
Latest commit de3e7d73caa1cf600178920c17b1f8459e605035
Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/639dac1eacea2e00097de5dd
netlify[bot] commented 1 year ago

Deploy Preview for react-daisyui processing.

Name Link
Latest commit de3e7d73caa1cf600178920c17b1f8459e605035
Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/639dac1eacea2e00097de5dd
netlify[bot] commented 1 year ago

Deploy Preview for react-daisyui ready!

Name Link
Latest commit de3e7d73caa1cf600178920c17b1f8459e605035
Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/639dac1eacea2e00097de5dd
Deploy Preview https://deploy-preview-276--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 settings.

sahinvardar commented 1 year ago

BTW it is worth noting that this is a breaking change

benjitrosch commented 1 year ago

You're right, it's causing more problems than it is helping. Best to remove it. I'll mark it as a breaking change.

sahinvardar commented 1 year ago

Thanks for merging. Any ETA on the next release with that change ?