search

daisyui / react-daisyui

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

fix(Stats): removed incorrect import on examples #339

Closed dev0T closed 1 year ago

dev0T commented 1 year ago

Adresses #338.

The examples were not using the library correctly, making use of internal imports.

netlify[bot] commented 1 year ago

Deploy Preview for react-daisyui processing.

Name Link
Latest commit 4377f2e48cd6698f98e784c991ccde0e9d2c260b
Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/643dca5d98b38f0008dbae29
netlify[bot] commented 1 year ago

Deploy Preview for react-daisyui processing.

Name Link
Latest commit 4377f2e48cd6698f98e784c991ccde0e9d2c260b
Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/643dca5d98b38f0008dbae29
netlify[bot] commented 1 year ago

Deploy Preview for react-daisyui ready!

Name Link
Latest commit 4377f2e48cd6698f98e784c991ccde0e9d2c260b
Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/643dca5d98b38f0008dbae29
Deploy Preview https://deploy-preview-339--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.

dev0T commented 1 year ago

Thanks for finding and fixing this. The subcomponents look a little cumbersome (and ugly) in this case so maybe worth revisiting how we handle the <Stats> component in the future.

We could probably combine Stat and Item into one component. Can't remember why they were ever separate components to begin with.

I will take a look into that today and see what I can do!

dev0T commented 1 year ago

After giving it a further look, I don't think combining Stat and Item is a good approach because you need to be able to provide classNames for the different parts of the Stat (Similar to the current lack of TableCell). I'd like to suggest an approach similar to NavBar sections where instead of providing Stats.Stat.Item along with a props variant, one would decide between Stats.Stat.Title, Stats.Stat.Value, etc.

I'd suggest merging this to correct the docs usage for now and after deciding the new component structure change it for a new version since it will also be a breaking change. What do you think @benjitrosch?

Edit: I created a PR so you can take a look! #341

benjitrosch commented 1 year ago

Thanks for looking into that. We can't update the docs at the moment since they're broken and are reverted to a previous release. So if it's just for the sake of updating the docs, we're unfortunately out of luck. On the bright side, that gives us time to come up with a better solution @dev0T 😅

dev0T commented 1 year ago

Thanks for looking into that. We can't update the docs at the moment since they're broken and are reverted to a previous release. So if it's just for the sake of updating the docs, we're unfortunately out of luck. On the bright side, that gives us time to come up with a better solution @dev0T 😅

I forgot about that issue! haha

I did create a new PR with my suggested approach, take a look whenever you have the time and let me know if you have any questions! #341

dev0T commented 1 year ago

Hey @benjitrosch! Since the docs aren't broken anymore, can this be merged? Thanks!