-
Notifications
You must be signed in to change notification settings - Fork 15
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
hackgreenville-com-432 - Set admin panel FilamentPHP icon, item sorting, and fix resource global searching #437
Draft
zach2825
wants to merge
10
commits into
hackgvl:develop
Choose a base branch
from
zach2825:hackgreenville-com-432
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c22c6a7
hackgreenville-com-432 - Set admin panel FilamentPHP icon, item sorti…
zach2825 b144f08
removing a relationship manager usage to do in git push
zach2825 cee17f6
hackgreenville-com-434 - Add events relationship to the orgs edit page
zach2825 1eb289e
Bump up asdf php version to php 8.3.6
zach2825 f89921f
fixed failing unit test by bringing over changes from another PR for …
zach2825 5d3b386
hackgreenville-com-433 - Add events widget to the dashboard
zach2825 401e7a4
Register the new widget to the admin dashboard
zach2825 5a94d70
Merge branch 'develop' of github.com:hackgvl/hackgreenville-com into …
zach2825 a4c8347
reverted the local toolset(asdf) php version
zach2825 76638d6
Merge branch 'develop' into hackgreenville-com-432
zach2825 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
45 changes: 45 additions & 0 deletions
45
app/Filament/Resources/EventResource/Widgets/TotalActiveEvents.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
<?php | ||
|
||
namespace App\Filament\Resources\EventResource\Widgets; | ||
|
||
use App\Models\Event; | ||
use Filament\Widgets\ChartWidget; | ||
|
||
class TotalActiveEvents extends ChartWidget | ||
{ | ||
protected static ?string $heading = 'Chart'; | ||
|
||
protected static ?string $description = 'All Events'; | ||
|
||
protected static ?string $maxHeight = '250px'; | ||
|
||
protected function getData(): array | ||
{ | ||
$data = Event::query() | ||
->published() | ||
->get() | ||
->groupBy('status') | ||
->map(fn ($events) => $events->count()) | ||
->toArray(); | ||
|
||
return [ | ||
'labels' => array_keys($data), | ||
'datasets' => [ | ||
[ | ||
'label' => 'Events', | ||
'data' => array_values($data), | ||
'backgroundColor' => [ | ||
'#FF6384', | ||
'#36A2EB', | ||
'#FFCE56', | ||
], | ||
], | ||
], | ||
]; | ||
} | ||
|
||
protected function getType(): string | ||
{ | ||
return 'pie'; | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to disagree with this approach - these icons should not live on the Model. This is very specific to the Filament dashboard. Just set the icon string here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogdankharchenko, the resources are directly tied to the models. Maybe I could register a config/icons.php file or something like that, but it's simpler to create consistency. For example, if we wanted to use the icon wherever that record is rendered, then we could just get the icon from the model. It's very easy to figure out the icon with that pattern.
I don't like having them hard-coded in the resource because it makes reuse harder and it makes maintaining them harder since their spread out throughout the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the idea of being able to update the icons in a config, but we have a bunch of different icons being used on the front-end in the navigation (header & footer), in a few pages, and in the homepage widgets.
If we're going to create configurable icons, then would it confuse the issue if we only did that for the admin panel and not the entire site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, but that icon configuration is specific for the filament resource not the model. It is not available outside of the filament dashboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with Bogdan on this one. We shouldn't mix concerns of the model with the view. Since we're only using this icon on Filament, we should be fine keeping the icon definition there. Until we actually run into a case where we need to use the same icon twice, we can figure it out then (which the icon config may not be a bad idea). Just don't think we should put the icon information on the model itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you're seeing the changes in the context of filamentPhp, but in reality, we can use these icons everywhere we display icons of this nature. It would help to create consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogdankharchenko and @irby
In open-source projects, decisions about icon usage are made more consistent by using model icon registers, config files, or lang packages, just to name a few ideas. I like Model icon registers because, IMO, they're the easiest to reason about, but really, I'm open to suggestions. I ask for reasoning because I might want to adopt the change personally.
Language packs would be neat because you can do different icons based on culture, like hats would be sombreros, buildings could be offal towers, etc. These are just examples and optional for hackgreenville.com. They are just giving options and alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a lot of problem defining icons like this. It's not a pattern I'm used to but I'm open to it. I'll defer to @bogdankharchenko on the final word for it. 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zach2825 since we're talking about a decent shift in implementation, how about we revert to what we had and move the icon flexibility as a task under our plans to upgrade the front-end framework?
We talked about exploring Tailwind, or upgrading to a newer Bootstrap in #252
Also, our current, older Bootstrap is causing issues like depreciation warnings we can't turn off without something like #451
So, we probably need a UI refresh for a number of reasons and the config-based icons could be part of that. Eric Anderson mentioned Iconpark in #ruby today and its more plentiful and style-able icons as a newer option, so that could be part of a larger revamp too if it justifies the need for more dynamic icons in the front-end.