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

feature: Configure UI icons with service provider #3568

Closed
wants to merge 1 commit into from

Conversation

zepfietje
Copy link
Member

@zepfietje zepfietje commented Aug 19, 2022

As Heroicons explains and Tailwind UI shows, the table filter and column toggle icons should use the 20px variants:

Also, I've updated the color to use the secondary color instead of primary, since they're not primary actions. Having a lot of buttons with the primary/CTA color is bad design practice.

Screenshots

Before

Screen Shot 2022-08-19 at 11 40 45

After

Screen Shot 2022-08-19 at 11 39 42

Icons to consider

  • Table filter trigger
  • Table column toggle trigger
  • Global search input
  • Sidebar open button
  • Dashboard page
  • Pagination buttons
  • Table column sort indicator
  • Table search input
  • User menu user item
  • User menu dark mode toggle
  • User menu light mode toggle
  • User menu sign out button

@zepfietje zepfietje added the enhancement New feature or request label Aug 19, 2022
@themahdavi
Copy link
Contributor

It's nice but i think outline varient is bettet than solid version but color change to secondary

@zepfietje
Copy link
Member Author

@themahdavi, that would require us to size the icons up from the current 20px to 24px, which looks way too bulky. As I stated in the PR description, the solid icons are designed for this purpose, the outlined are not.

@ryangjchandler
Copy link
Member

I have to agree that the outline variants look a lot better than the solid ones..

@zepfietje
Copy link
Member Author

@ryangjchandler, I really have to disagree.

I do understand that some people may have preference for outlined icons over solid ones, but the outlined icons are just not designed to be rendered at the 20px size. The current downscaled outlined icons look awkward at that size. If you look at the comparison in this PR description, you'll also see that when comparing them to the 20px solid icons.

@danharrin
Copy link
Member

I also agree with @themahdavi and @ryangjchandler 😬 Just because an icon was designed for a specific size doesn't mean it doesnt work in other styles, because it's all contextual.

However, I agree that we can probably make them secondary instead of primary.

@zepfietje
Copy link
Member Author

I wish design choices were easier to explain...

Totally understand your point about context.
However, the outlined icons at 20px really don't look right here in my opinion.

The alignment is bad at that size.
Just look at the height of the outlined columns icon compared to the filter icon next to it.
Feels really awkward, whereas the relative sizing is perfect on the solid icons.
That's exactly one of the reasons why these icons are designed to work at respectively 24px and 20px size.

Also, using the outlined icons in this situation and at the 20px size contributes to the inconsistency across the entire project UI. It may sound strange, but that really adds up. These little design choices might look ok on their own, but when viewed in the broader scope of the project it leads to a noticeably worse design/UI.

Don't get me wrong; it's not about outlined vs. solid icons.
It's mostly about the correct sizing; and the intended size they're designed to be rendered at certainly plays a big role as explained above.

@danharrin
Copy link
Member

So how does it look if we work out how to make the outlined icons work at the larger size?

@margarizaldi
Copy link
Contributor

margarizaldi commented Aug 22, 2022

Why don't simply make ->fiterIcon('...'), ->filterIconColor(...), ->columnToggleIcon('...'), and ->columnToggleIconColor ?

And these subjective debates will end soon 😄

Actually I want to do it but I don't really know how to make a good and acceptable PR... Even I'm still confused how to use git, github, fork, branch, merge, etc... What a sad story... 😭

@zepfietje
Copy link
Member Author

zepfietje commented Aug 22, 2022

Good idea, @margarizaldi!
Actually I'd been thinking about this exact idea just yesterday.

I think it'll be a good solution for icons in other places of Filament as well.
This will allow us and users of Filament to normalize their icon sizes and colors.

@margarizaldi
Copy link
Contributor

@zepfietje but as we also need consistency to our potentially big app, what about to also have those and other common icons file inside the config\filament.php or separated config file.

Some icons to consider are:

  • Table filter trigger
  • Table column toggle
  • Create, edit, view and delete action button
  • Submit & cancel button
  • Table search

And for those especially who want to use another icon pack:

  • Dashboard/home
  • Global search
  • User
  • Dark/light mode

@zepfietje
Copy link
Member Author

We're moving away from config items and rather configure such things in a service provider.

For action buttons it's already easy to configure the icon with configureUsing.
I'll create a list of icons in this PR description and will think about a nice solution.

@danharrin
Copy link
Member

We should have an IconsRepository class that is bound to the container & registers icons for all parts of Filament

@danharrin danharrin changed the title Improve trigger icons Configure UI icons with service provider Aug 22, 2022
@danharrin danharrin marked this pull request as draft August 22, 2022 20:54
@dmandrade
Copy link
Contributor

What about an icon object, something like:

Icon::config('filter')->use('heroicon-s-filter')->size('large')->color('primary')

We can use this to configure icons in service provider and others locations:

$table->filterIcon('heroicon-s-filter') OR $table->filterIcon(fn ($icon) => $icon->use('heroicon-s-filter')->color('primary'))

I think is better than several methods like filterIcon, filterColor, etc...

@zepfietje
Copy link
Member Author

Not the exact API you describe, but something similar I coined once. Thanks for the comment, since I think it would be quite neat to have an Icon class/component.

@ralphjsmit
Copy link
Contributor

ralphjsmit commented Aug 29, 2022

Not the exact API you describe, but something similar I coined once. Thanks for the comment, since I think it would be quite neat to have an Icon class/component.

If you go the route of using an IconRepository, would it be an idea to add an option to switch between outlined and solid in bulk? E.g sth like an ->outline() and ->solid() method, perhaps on a HeroiconRepository extends/implements IconRepository.

@zepfietje
Copy link
Member Author

Not the exact API you describe, but something similar I coined once. Thanks for the comment, since I think it would be quite neat to have an Icon class/component.

If you go the route of using an IconRepository, would it be an idea to add an option to switch between outlined and solid in bulk? E.g sth like an ->outline() and ->solid() method, perhaps on a HeroiconRepository extends/implements IconRepository.

I don't think that would be within the scope of Filament as we allow you to use any icon set you like (or really any Blade component). Switching manually isn't too difficult, right? Just search heroicon-o in your code and replace it with heroicon-s?

Maybe something for the future, but let's focus on the basics for now.

@ralphjsmit
Copy link
Contributor

I don't think that would be within the scope of Filament as we allow you to use any icon set you like (or really any Blade component). Switching manually isn't too difficult, right? Just search heroicon-o in your code and replace it with heroicon-s?

Maybe something for the future, but let's focus on the basics for now.

That would depend on the implementation you choose. I can't do a find and replace in the vendor folders. If you really want to switch to the solid icons by default, it'd be nice to be able to switch back to the outline ones easily.

@zepfietje
Copy link
Member Author

I don't think that would be within the scope of Filament as we allow you to use any icon set you like (or really any Blade component). Switching manually isn't too difficult, right? Just search heroicon-o in your code and replace it with heroicon-s?
Maybe something for the future, but let's focus on the basics for now.

That would depend on the implementation you choose. I can't do a find and replace in the vendor folders. If you really want to switch to the solid icons by default, it'd be nice to be able to switch back to the outline ones easily.

There are no plans to switch to solid icons by default.

@danharrin
Copy link
Member

danharrin commented Aug 29, 2022

I basically just want a way where you can call stuff from a service provider

Icon::for('tables::search')
    ->is('heroicon-o-search')
    ->size('w-5 h-5')

@danharrin
Copy link
Member

Because it's important to be able to change icon width as well as swap icons out

@ralphjsmit
Copy link
Contributor

ralphjsmit commented Aug 29, 2022

I basically just want a way where you can call stuff from a service provider

Icon::for('tables::search')
    ->is('heroicon-o-search')
    ->width('w-5 h-5')

Allright, that sounds good!

When I read about an IconRepository, I imagined something like the below, where you could create several implementations for icons. By default a HeroiconsRepository, but e.g. a TablerIconsRepository could be contributed by the community in a package or sth.

If you're going that route, an ->outline() and ->solid() makes sense, so people can easily choose the version they prefer.

<?php

abstract class IconRepository
{
    protected $lookupTable = [
        //
    ];

    public function getIcon(string $key): ?string
    {
        return $this->getLookupTable()[$key] ?? null;
    }

    public function getLookupTable(): array
    {
        return $this->lookupTable;
    }
}

class HeroiconRepository extends IconRepository
{
    protected $lookupTable = [
        'tables::search' => 'search',
        // ...
    ];

    protected string $version = 'outline';

    public function getLookupTable(): array
    {
        return collect($this->lookupTable)
            ->map(function(string $icon) {
                $version = match($this->version) {
                    'solid' => 's',
                    'outline' => 'o'
                };

               return "heroicon-{$version}-{$icon}";
            })
            ->all();
    }

    public function solid(): static
    {
        $this->version = 'solid';

        return $this;
    }

    public function outline(): static
    {
        $this->version = 'outline';

        return $this;
    }
}

@zepfietje
Copy link
Member Author

I basically just want a way where you can call stuff from a service provider

Icon::for('tables::search')
    ->is('heroicon-o-search')
    ->size('w-5 h-5')

What about actually specifying the icon as an object?

Icon::make('heroicon-o-search')
    ->size('sm')
    ->color('danger')

@ralphjsmit
Copy link
Contributor

ralphjsmit commented Aug 29, 2022

What about actually specifying the icon as an object?

Hmm, that's also nice. If you implement Stringable, you'd even be able to echo it out in Blade {!! Icon::make('heroicon-o-search')->size('sm') !!}.

@zepfietje
Copy link
Member Author

What about actually specifying the icon as an object?

Hmm, that's also nice. If you implement Stringable, you'd even be able to echo it out in Blade {!! Icon::make('heroicon-o-search')->size('sm') !!}.

Don't need to implement Stringable for that. We can extend our ViewComponent class like we do for stuff like actions and notifications already.

@ralphjsmit
Copy link
Contributor

Don't need to implement Stringable for that. We can extend our ViewComponent class like we do for stuff like actions and notifications already.

That's also great and accomplishes the same goal. Wasn't aware about the specific implementation details for Actions and Notifications.

@danharrin
Copy link
Member

danharrin commented Aug 29, 2022

I don't think we should include different sets of icons. Those can be registered through plugins using the Icon facade that I provided above. Outline / solid is Heroicon-specific, I don't see any reason to add that as a built-in feature.

@margarizaldi
Copy link
Contributor

Does this discussion also consider about light & dark theme? Since I don't think outline icons work best for dark theme, they look weird, too bold, and ugly. While they're cool and very clear for the light theme.

And fortunately there's phosphor icon which has duotone version that works best for both theme, so I can use every icon confidently in my project.

I hope that the new icon class or whatever it will be, gives us flexible options either to use different or the same icon for dark & light mode.

@saade
Copy link
Member

saade commented Aug 30, 2022

The idea of having an icon repository is really good! Since i'm planning switching all the heroicons with another set.

For that, would be great if we can provide all icons at once (a lookupTable or smth), to avoid writing Icon::for(...) for hundreds of icons.

That will make easier for people to contribute with another set of icons via a plugin too.

@zepfietje
Copy link
Member Author

Does this discussion also consider about light & dark theme? Since I don't think outline icons work best for dark theme, they look weird, too bold, and ugly. While they're cool and very clear for the light theme.

And fortunately there's phosphor icon which has duotone version that works best for both theme, so I can use every icon confidently in my project.

I hope that the new icon class or whatever it will be, gives us flexible options either to use different or the same icon for dark & light mode.

I'm not sure if you should switch icons completely for dark and light theme. Let's stick to the basics for this PR and see if it's something we'd like to add later.

@zepfietje zepfietje changed the title Configure UI icons with service provider feature: Configure UI icons with service provider Sep 2, 2022
@danharrin danharrin linked an issue Sep 17, 2022 that may be closed by this pull request
@zepfietje zepfietje added this to the v2 milestone Oct 7, 2022
@danharrin danharrin modified the milestones: v2, v3 Oct 7, 2022
@danharrin danharrin closed this Oct 7, 2022
@danharrin
Copy link
Member

Closed as this has not been worked on and will be implemented in v3 instead.

@danharrin danharrin removed this from the v3 milestone Oct 7, 2022
@zepfietje zepfietje deleted the improve-trigger-icons branch October 8, 2022 10:02
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants