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 HeroIcon class feature #10984

Closed
wants to merge 16 commits into from
Closed

add HeroIcon class feature #10984

wants to merge 16 commits into from

Conversation

ArielMejiaDev
Copy link

Description

The Issue

Filament is a super productive tool, and reducing the necessity of checking in the Heroicons page for icon names would be a great addition to the workflow.

Goal
Increase developer experience by removing the necessity to set icons as strings.

Additions
It adds the ability to handle Heroicons with autocompletion using a new class with a static constructor, constants for every icon available.

Variants
By default the icon variants are outline, but it adds the ability to chain method solid to set an icon as solid

This new feature would be ready to use with:

  • Resource Navigation Icon
  • Form Fields (hintIcon, prefixIcon, suffixIcon)
  • BulkAction (icon)
  • Notification (icon)

Usage

// Usage in Navigation Icons
public static function getNavigationIcon(): string|Iconizable|null
{
    return HeroIcon::make(HeroIcon::USERS);
}

// Usage in Forms

// Basic Usage
Forms\Components\TextInput::make('name')
    ->hintIcon(HeroIcon::make(HeroIcon::IDENTIFICATION))

// In a form as prefix
Forms\Components\TextInput::make('email')
    ->prefixIcon(HeroIcon::make(HeroIcon::ENVELOPE))

// In a form as suffix
Forms\Components\TextInput::make('password')
    ->suffixIcon(HeroIcon::make(HeroIcon::LOCK_CLOSED)),


// Usage in Table Actions & Notifications
Tables\Actions\Action::make('remove')
    ->icon(HeroIcon::make(HeroIcon::TRASH))
    ->action(function () {
        return Notification::make()
            ->warning()
            ->icon(HeroIcon::make(HeroIcon::INFORMATION_CIRCLE))
            ->title('Notification Title')
            ->body('Notification body text...')
            ->send();
    }),

// Usage in BulkActions
Tables\Actions\DeleteBulkAction::make()
    ->icon(HeroIcon::make(HeroIcon::RECTANGLE_STACK)),

Resource Stub

The resource stub file has been updated to replace the $navigationIcon property, which still works, with this new method to resolve the icon fluently.

Testing

All resource tests in tests/src/Panels/Resources directory are updated to work with this feature

  • Changes have been tested.

Demo

https://www.loom.com/share/614ce4b4b5ae40dba8f5ac0b07f96472?sid=ad0e224e-4547-4839-b115-55f51adcd03b

Documentation

If this feature could be considered to be approved, I could gladly add a PR in docs too.

@zepfietje
Copy link
Member

Changing the type of the navigation icon property is a breaking change.
Hence why it's planned for v4: #10281.

@zepfietje zepfietje closed this Jan 22, 2024
@zepfietje zepfietje added this to the v3 milestone Jan 22, 2024
@zepfietje zepfietje added the enhancement New feature or request label Jan 22, 2024
@danharrin danharrin reopened this Jan 22, 2024
@danharrin
Copy link
Member

I've been thinking about #10281, and I think that we don't need an Icon class or Enum. I think just including a Heroicon class with a constant for each icon string is good enough, as it provides autocomplete while also not making any breaking type changes.

@ArielMejiaDev please remove all type changes, and put all possible Heroicon combinations as constants in a Heroicon class. Then we can just get people to use those without any breaking changes:

$navigationIcon = Heroicon::O_PLUS;
$navigationIcon = Heroicon::S_PLUS;
$navigationIcon = Heroicon::M_PLUS;

@danharrin danharrin linked an issue Jan 22, 2024 that may be closed by this pull request
@zepfietje
Copy link
Member

Makes sense, @danharrin.
Regarding naming, do we want to use O_PLUS or OPlus like we do for colors?

@danharrin
Copy link
Member

Since these are class constants instead of enum cases, we should probably go with UPPER_SNAKE. Can't remember why colors are that way but I think they're ok.

@ArielMejiaDev
Copy link
Author

@danharrin thanks I am going to add the changes

@danharrin danharrin self-assigned this Jan 22, 2024
@ArielMejiaDev
Copy link
Author

@danharrin The HeroIcon class now has all icon variants as constants, at this point, it should not break anything for V3, for V4 I would be glad to help if there are plans for an icon more complex implementation.

In any way this constants are going to be pretty helpful

@danharrin
Copy link
Member

Thanks!

Do you think it would be confusing that the docs / stubs / Filament core code still use the old strings instead of the constants? Should they be updated?

@ArielMejiaDev
Copy link
Author

@danharrin I could make a PR to update the docs to use the class constants

For stubs, you can choose between keep it or replace it, for navigation icons it would require to override a method instead of the property, but both works without any breaking change.

I could imagine my workflow with the class constants and I think most of developers would prefer it, avoid typos, magic number issues, etc, let me know your thoughts, I would be glad to help in any case

@zepfietje
Copy link
Member

zepfietje commented Jan 24, 2024

Can't the constants be used on icon property initialization, @ArielMejiaDev?
If not, I don't see a big reason to use constants instead of enums, @danharrin.

Also, benefit of enums is that one can list all available icons easily using ::cases().
If we're going to have to use the methods (like getNavigationIcon()) instead of the props, we might as well go for something like Heroicon::User->outline(), which is much more readable in my opinion.

@danharrin
Copy link
Member

As far as I know, there is no issue using constants for property initialisation.

@danharrin
Copy link
Member

Screenshot 2024-01-24 at 09 58 26

@ArielMejiaDev
Copy link
Author

For sure, so if you are fine with this I can add the change in the stubs and in the docs

@danharrin
Copy link
Member

Yes please

@danharrin
Copy link
Member

And anywhere we reference Heroicon names in Filament internal code

@ArielMejiaDev ArielMejiaDev closed this by deleting the head repository Jan 27, 2024
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
Archived in project
Development

Successfully merging this pull request may close these issues.

Introduce Heroicon enum
3 participants