hackgreenville-com-432 - Set admin panel FilamentPHP icon, item sorting, and fix resource global searching #437
Conversation
…ng, and fix resource global searching resolves hackgvl#432
| @@ -14,7 +15,9 @@ class CategoryResource extends Resource | |||
| { | |||
| protected static ?string $model = Category::class; | |||
|
|
|||
| protected static ?string $navigationIcon = 'heroicon-o-rectangle-stack'; | |||
| protected static ?string $navigationIcon = Category::icon; | |||
There was a problem hiding this comment.
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.
@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.
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.
@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
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.
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.
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.
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.
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.
@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.
| @@ -20,131 +25,164 @@ class OrgResource extends Resource | |||
|
|
|||
| protected static ?string $model = Org::class; | |||
|
|
|||
| protected static ?string $navigationIcon = 'heroicon-o-rectangle-stack'; | |||
| protected static ?string $navigationIcon = Org::icon; | |||
There was a problem hiding this comment.
See reply here -
#437 (comment)
| @@ -14,7 +14,9 @@ class StateResource extends Resource | |||
| { | |||
| protected static ?string $model = State::class; | |||
|
|
|||
| protected static ?string $navigationIcon = 'heroicon-o-rectangle-stack'; | |||
| protected static ?string $navigationIcon = State::icon; | |||
There was a problem hiding this comment.
See reply here -
#437 (comment)
| @@ -14,39 +15,43 @@ class TagResource extends Resource | |||
| { | |||
| protected static ?string $model = Tag::class; | |||
|
|
|||
| protected static ?string $navigationIcon = 'heroicon-o-rectangle-stack'; | |||
| protected static ?string $navigationIcon = Tag::icon; | |||
There was a problem hiding this comment.
See reply here -
#437 (comment)
| @@ -14,7 +15,9 @@ class CategoryResource extends Resource | |||
| { | |||
| protected static ?string $model = Category::class; | |||
|
|
|||
| protected static ?string $navigationIcon = 'heroicon-o-rectangle-stack'; | |||
| protected static ?string $navigationIcon = Category::icon; | |||
There was a problem hiding this comment.
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.
resolves #432