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

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

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

dev0T
Copy link
Contributor

@dev0T dev0T commented Apr 17, 2023

Adresses #338.

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

@netlify
Copy link

netlify bot commented Apr 17, 2023

👷 Deploy Preview for react-daisyui processing.

Name Link
🔨 Latest commit 4377f2e
🔍 Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/643dca5d98b38f0008dbae29

1 similar comment
@netlify
Copy link

netlify bot commented Apr 17, 2023

👷 Deploy Preview for react-daisyui processing.

Name Link
🔨 Latest commit 4377f2e
🔍 Latest deploy log https://app.netlify.com/sites/react-daisyui/deploys/643dca5d98b38f0008dbae29

@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for react-daisyui ready!

Name Link
🔨 Latest commit 4377f2e
🔍 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.

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.

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.

@dev0T
Copy link
Contributor Author

dev0T commented Apr 21, 2023

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
Copy link
Contributor Author

dev0T commented Apr 21, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

dev0T commented Apr 22, 2023

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
Copy link
Contributor Author

dev0T commented Jun 9, 2023

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

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.

2 participants