Introduce Heroicon enum

[zepfietje]

Targeting v4, since changing the property type on e.g. resources is a breaking change.

Donate 💰 to fund this issue

• You can donate funding to this issue. We receive the money once the issue is completed & confirmed by you.
• 100% of the funding will be distributed between the Filament core team to run all aspects of the project.
• Thank you in advance for helping us make maintenance sustainable!

[ryangjchandler]

I'm not sure a Heroicon abstraction is the right thing... a more generic Icon abstraction might make more sense, then Filament can provide a Heroicon implementation of Icon.

[danharrin]

@ryangjchandler Any BackedEnum should work I think

[ryangjchandler]

@ryangjchandler Any BackedEnum should work I think

Yeah or a BackedEnum, depends on whether you want to have any methods for doing things like default size etc.

[awcodes]

What is the actual feasibility of maintaining an enum with 80 or 90+ cases? Or am I just missing the point?

[ryangjchandler]

We could generate the enums with some codegen, that wouldn't be too difficult. We could also add a small script to the support package or something to let users generate their own ones.

[tembra]

I think that the type should not be changed, but BackedEnum added to it: string | \BackedEnum.

This way we do not need to target v4 because it will not be a breaking change. Please correct me if I'm wrong.

I think we can even go further implementing something like this:

/**
 * implementation
 */

// specific enum for each package and type
enum HeroiconOutline: string
{
    case x_circle = 'heroicon-o-x-circle';
}

// enum for a specific package types
enum HeroiconType: string
{
    case outline = 'o';
    case solid = 's';
}

// enum for a package with a default type implemented
interface HasIconLabel
{
    public function getIconLabel(?\BackedEnum $type): ?string;
}

enum Heroicon: string implements HasIconLabel
{
    case x_circle = 'x-circle';

    public function getIconLabel(?\BackedEnum $heroiconType): ?string
    {
        // default type. we can get it from a configuration like Filament::getDefaultIconType()
        $heroiconType ??= HeroiconType::outline;

        $type = $heroiconType->value;

        $icon = $this->value;

        return "heroicon-$type-$icon";
    }
}

// the new icon function
function icon(string | \BackedEnum $name, ?\BackedEnum $type = null): ?string
{
    $icon = $name;

    if ($name instanceof \BackedEnum) {
        $icon = $name->value;

        if (method_exists($name, 'getIconLabel')) {
            $icon = $name->getIconLabel($type);
        }

        if (! is_string($icon)) {
            return null;
        }
    }

    return $icon;
}

/**
 * usage examples
 */

// direct string like used now still works
echo icon('heroicon-o-x-circle'); // heroicon-o-x-circle
echo "\n";

// specific enum
echo icon(HeroiconOutline::x_circle); // heroicon-o-x-circle
echo "\n";

// package enum + default type
echo icon(Heroicon::x_circle); // heroicon-o-x-circle
echo "\n";

// package enum + type
echo icon(Heroicon::x_circle, HeroiconType::solid); // heroicon-s-x-circle
echo "\n";

[ryangjchandler]

@tembra This would still be a breaking change unfortunately, since inherited property types are "invariant", which means the type cannot change during inheritance. Adding a new type to the union would break all existing applications - an example of this would be the $navigationIcon property on Resource classes.

It would be okay if the getNavigationIcon() method for example was the only place that a type hint existed since return types are "covariant", which means the type can be narrowed (made more specific) during inheritance.

So sadly, this will have to be a change made in 4.x.

[tembra]

@ryangjchandler Oh yes, of course! I'm new to Filament and totally forgot that some icons can be defined in a class property.

What do you all think to make this implementation in two steps?

1. On icon's methods first in the current v3
   • to let everyone begin to adopt this new way
   • to also encourage them to use Enum::case->value in class properties if still used
2. On icon's class properties in v4
   • The breaking change for the earlier adopters will be simply remove the ->value from Enum::case->value

I also think that would be nice to deprecate the usage of class properties for icons (maybe for everything?) to do not have problems like this ever again. But to do this we will need to add abstract methods for every class property and also maintain the classes properties (and their getters) probably along two versions when, perhaps in v5, we would finally remove the entire classes properties.

[andrewdwallo]

What are all of the cons if an Enum was introduced for something like this? Is there any difference in performance/load-time when using what we have currently (e.g. a string) vs using this proposition? And I mean when testing and comparing the two with an application that uses a significant amount of heroicons.