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

Add empty state to chart widget #13408

Draft
wants to merge 5 commits into
base: 3.x
Choose a base branch
from

Conversation

juliangums
Copy link
Contributor

Description

Caution

DON'T MERGE THIS YET!

@danharrin as discussed in #13311 I have added a feature to display an empty state on chart widgets. Before this is merged, I'd like to ask a couple of things as I am not entirely familiar with everything in Filament:

  1. I added a string and translations for en and de. Is that enough? How are all the other translations handled for so many languages?
  2. I didn’t use screenshots or links in the docs as I didn’t know how to do them.
  3. The implementation is very much based on the work done on tables. But as we aren't defining these things by calling methods on an object but rather by defining a class that extends ChartWidget, I removed all the setters from it. But I guess that also means closures don't really make sense for the attributes or do they? I want to make sure this is in the spirit of Filament so any guidance would be much appreciated.
  4. Is everything else as you'd expect? Anything I missed?

Let me know and I'll get the rest sorted.
Otherwise, it's ready to be tested.

Visual changes

Screenshot 2024-06-26 at 23 22 49

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@zepfietje zepfietje changed the title feature: add empty state to chart widgets Add empty state to chart widgets Jun 27, 2024
@zepfietje zepfietje changed the title Add empty state to chart widgets Add empty state to chart widget Jun 27, 2024
Copy link
Member

@zepfietje zepfietje 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 this PR, @juliangums!

I don't like the duplication of empty state Blade views.
Since I was already planning to extract the empty state component from tables, shall we get #13252 done before merging your new feature?

@zepfietje zepfietje added the enhancement New feature or request label Jun 27, 2024
@zepfietje zepfietje added this to the v3 milestone Jun 27, 2024
@juliangums
Copy link
Contributor Author

@zepfietje that would make a ton of sense. Let me know when that’s done or if you need any help just instruct me.

@zepfietje
Copy link
Member

Great! It's at the top of my list, so will get to it as soon as possible.

I'll mark this PR as draft until we can move forward. 👍

@zepfietje zepfietje marked this pull request as draft June 27, 2024 07:55
@zepfietje zepfietje self-assigned this Jun 27, 2024
@zepfietje zepfietje linked an issue Jul 19, 2024 that may be closed by this pull request
@zepfietje
Copy link
Member

Started working on #13642.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants