Refactor to Blade Icons

[driesvints]

I'm very happy to say that Adam was so kind to let me take over the library. I recently created my own Blade Icons package without realizing that Adam already created this wonderful package! After thinking it over I thought it would be perfect if we merged the two packages into one. So that's great news for everyone using this library as I'll be actively maintaining it and helping out with issues etc.

There's some pointers on this so let's go through them. First of all, this PR is a complete rewrite of the package. If I'm going to maintain it and use it as a base for future packages/ideas I'd like to be familiar and comfortable with the library so a rewrite makes sense. You can check out the changes in this pr, the new README on its own branch as well as the upgrade guide.

New Requirements

There's some new minimum version requirements like PHP 7.2 and Laravel 7.14 (I need this version specifically for the Blade components). It doesn't makes sense to me to maintain anything for a PHP version that isn't getting active support anymore.

New Features

There's new stuff as well! The package now supports multiple icon sets which makes it very easy for third party packages to be created for it. When the refactor has been merged and released I'll release another "Heroicons" package alongside it which you can optionally pull in.

Besides that you can now reference icons as Blade components. This is a very clean syntax and I personally much prefer this over a directive.

Breaking Changes

There's quite a few breaking changes unfortunately but making these changes now before a full 1.0 release is done will allow me to work with a syntax that I'm comfortable with. Please refer to the upgrade guide for the most notable ones.

Package Renaming

One last note I want to make is that this rewrite is part of a bigger plan that I have. Before this PR is merged I'll transfer the repo to a new organisation and rename its package name. I'll give more info as soon as we get to that.

I need your help

What I need most now is feedback and people testing the library, especially existing users. I'd much appreciate it! Try out this PR by installing it through Composer:

composer require nothingworks/blade-svg:dev-next-version

There's two things I'm currently considering before shipping the next 0.4.0 release:

• Should we remove the sprite sheets functionality? It doesn't seems to be like something many people use and would clean up the internals a lot. (Sprite Sheets functionality was removed)
• At the moment blade components icons are loaded with loading the file list on every request. It's probably best that we cache this somehow between requests.

I'd much appreciate feedback so let me know what you think!

[avvertix]

Hi @driesvints that's a wonderful new life for this library. Thanks for accepting comments on the topic.

New features like multiple iconset will make a valuable addition especially for defining also illustrations and not only icons. I had a requirement like that when creating an application and I almost created blade components (using the old syntax) to solve it for each illustrations as I was already pulling in an iconset with blade-svg.

There's some new minimum version requirements like PHP 7.2 and Laravel 7.6

The new Blade components are definitely a good point to break compatibility, but old Blade directives are still valuable for who uses Laravel 6.x (which if I'm correct is a LTS). I personally think that supporting an LTS version is an option to consider, or at least create an intermediate version that supports it.

At the moment blade components icons are loaded with loading the file list on every request. It's probably best that we cache this somehow between requests.

I think this was also the approach @adamwathan took. Indeed you have to read the file somewhere unless you cache the entire view in html. Maybe a mad idea could be inlining the SVG in the view cache already generated by Blade instead of the method to read the file. I honestly have no idea how this could be done, but using file cache or redis or the database would not resolve the problem, as it will just read a different "file".

You can check out the changes in this pr, the new README on its own branch as well as the upgrade guide.

I like the approach for getting icons from different sets by prefixing with the set name.

What if I would like to deliver my iconset package via composer?

Let me explain, I created a package avvertix/materialicons-laravel-bridge which was using @adamwathan blade-svg under the hood to deliver the MaterialIcons iconset. Maybe not the most elegant way, but it worked out. Considering your will to rewrite and expand with more iconsets I would love to hear your opinion on additional packages that delivers iconset extensions. Something like :

composer require blade-icons-awesome-set

'sets' => [
    'awesome-set' => [
        'path' => \AwesomeSet\Providers\Icons::class,
    ],
],

and \AwesomeSet\Providers\Icons::class is an invokable class returning the path on disk.

Thanks again @driesvints to be open to accept comments.

[driesvints]

Hey @avvertix, thanks a bunch for the very thorough feedback!

I personally think that supporting an LTS version is an option to consider, or at least create an intermediate version that supports it.

Unfortunately we can't do that. I'm making use of a feature in Laravel 7.6 for the Blade components so that's going to have to be the very minimum I'm afraid.

Indeed you have to read the file somewhere unless you cache the entire view in html.

I already took over the same in-memory caching approach that adam dit for individual svg contents but I'm more talking about the new file-listing that's done when the Blade components are registeren. Maybe we need to create like a manifest. But I'm not sure if it's a big enough problem to worry about.

Maybe a mad idea could be inlining the SVG in the view cache already generated by Blade instead of the method to read the file.

Hmm, yeah I'm also not sure how that'd be tackled.

What if I would like to deliver my iconset package via composer?

I'll write a dedicated section on this in the readme soon! After I got some initial feedback here I'm gonna build the heroicons packages. It's actually really easy as all you need to do is to boot the icon set in your service provider.

[minthemiddle]

Should we remove the sprite sheets functionality? It doesn't seems to be like something many people use and would clean up the internals a lot.

Totally, never used it myself nor in my teams.

[driesvints]

@minthemiddle thanks. As it stands and from all the feedback I've gotten so far I'm probably going to remove the sprite sheets support this weekend.

[mvdnbrk]

I have used this package in the past. Never used the sprite sheets as well.
Good to hear this package gets some fresh air 👍

Came back here for a new project I am working on. Will pull in the PR to try it out.
Thanks Dries!

[driesvints]

Heya, some updates:

• I've decided to remove sprite sheets functionality as I get the feeling that they're not much used at all.
• Added a section about how to build a third party package for your icons.

I feel like we're pretty close to wrapping things up now. The most thing that's needed atm is people testing it.

[njoguamos]

I have been using this package for quite sometimes. I have the dev-next-version a spin and here are my quick thoughts.

1. Though renaming svg_image() helper to svg() is a breaking change, I support it, as a small price to pay 😀.
2. I totally support removal of sprite sheets, I personally never used theme before.
3. The new blade components comes in handy. However I am having problem loading icons with it.

{{-- This is working --}}
@svg('search','h-4 w-4'')

{{-- This is working too --}}
{{ svg('search','h-4 w-4') }}

{{-- This is NOT working --}}
<x-icon-search class="h-5 w-5"/>

Am getting an error Unable to locate a class or view for component [icon-search]. I will I investigate my codebase and open an issue if necessary and share details.

4. Finally, am quite interested on how caching icons can be achieved. Am however happy to use the package to render svg and focus on optimising performance on other key areas such as database queries.

@driesvints keep pumping life to this package.

[bambamboole]

@driesvints I tried to use the package inside another laravel package and it worked out of the box with a custom set.
Can't wait fo the new release. Good work!

[driesvints]

@agentphoenix @njoguamos okay, I've managed to fix the default classes and I've also added the ability to add default classes per-set. Make sure to re-publish your config files when testing.

Managed to pull the latest commit. Unfortunately, the blade component is not working.

Hmm, that's strange. Your code samples are working for me. Did you clear your views? You're sure your icons are place in the correct directory and named correctly? Can you republish your config file? Did you uncomment the options inside?

[njoguamos]

@driesvints I will give it another try and let you know.

[agentphoenix]

@driesvints Works like a charm! Thanks for the quick turnaround on this.

[driesvints]

Unless there's any more feedback I'm probably going to be moving the package to its new location and doing the 0.4.0 release somewhere next week.

[njoguamos]

@driesvints I re-published the config file, cleared view and pulled the latest commit

Hmm, that's strange. Your code samples are working for me. Did you clear your views? You're sure your icons are place in the correct directory and named correctly? Can you republish your config file?

Did you uncomment the options inside?

My config

return [
    'sets' => [
        'default' => [
            'path' => 'resources/icons',
            'prefix' => 'icon',
            'class' => 'h-5 text-gray-130 fill-current',
        ],
    ],

    'class' => '',
];

Everything is working as expected except the blade component. The default set class h-5 text-gray-130 fill-current is not being applied on blade component.

// This applies
@svg('icon-globe')

// This applies
{{ svg('icon-globe') }}

// This renders svg but doesn't apply 
<x-icon-globe></x-icon-globe>

// This applies
<x-icon-globe class="h-5 fill-current text-gray-130"></x-icon-globe>

And thank for making the blade components work. Effort appreciated.

[agentphoenix]

I ran into a problem tonight running my test suite in Github Actions with this package. From the exceptions thrown, it seems that the package can't properly parse out the set from the icon name. Since I don't use a set named default, my tests blow up saying it can't find the icon fluent-eye-off in the set default. Instead, it should be looking for the icon eye-off in the set fluent.

Everything works fine locally and on the test server I'm running, but Github Actions throws an exception the first time I do $this->get from my feature test. Any ideas why that might be happening? Any ideas for how to best debug this?

[driesvints]

I'll take a look at these issues tomorrow and will try to solve them. Thanks a lot for testing btw!

If everything goes well I might also finally move the repo and release the new version tomorrow.

[driesvints]

Hey @njoguamos. Thank you so much for helping with testing. I pushed a commit that will fix this. You might have to re-run composer install and clear your views to test again.

@agentphoenix hmm I can't reproduce that. I've added two tests to properly test the scenario without a default set present: 8d531f4

Can you maybe share the code to reproduce this?

[agentphoenix]

@driesvints Unfortunately I can't reproduce the issue in my local tests, on my local server, or on my staging server. When I run the test suite through Github Actions though, it blows up. Here's a link to the Github Action result where you see the exception.

https://github.com/anodyne/nova3/runs/780876889?check_suite_focus=true

I'm just more curious about ways I can debug what's actually happening with GA so that I can figure out how to reproduce this locally and see if it's something with my app or with the package.

[driesvints]

@agentphoenix I see what's wrong. You're not using your set prefix when referencing the icons: https://github.com/anodyne/nova3/blob/dev/nova/resources/views/components/input/password.blade.php#L20

I think you missed this part in the readme:

When referencing icons with the Blade directive or helper you can omit the prefix to reference icons from the default set.

I'll try to make that even more clear by adding a sentence that says the prefix is mandatory for other sets.

[agentphoenix]

@driesvints So I have a custom directive @icon that I use since users can swap the icon set they use in the app. Behind the scenes, it takes the name passed into the directive, does a lookup on an icon set map, and then prepends the set prefix and calls the Svg class. The same error gets thrown if I do @svg('fluent-eye-off') as well.

[driesvints]

@agentphoenix we can't offer support for custom directives sorry.

The same error gets thrown if I do @svg('fluent-eye-off') as well.

Hmm that's odd because this test verifies that it should work: 8d531f4#diff-b0e3204a285e6556082196cf2a8dc305R125

[driesvints]

Hey everyone. We've finally merged this and moved the package to its new home. I'll be leaving this branch on this repo for a few weeks but it's best that you move to the stable 0.4.0 tag as soon as possible.

Thanks a lot for helping with testing!

[agentphoenix]

@driesvints Yeah no, I wouldn't expect support for the custom directive. It's just weird because everything is being built up properly and even with the @svg directive the tests are failing, so I'm just at a loss for how to debug it. No worries though, I'll keep hammering on it. Thanks for all your work on this!

[njoguamos]

@driesvints thank you for taking your time to refine the package.

@agentphoenix I understand what you are facing. I have something similar. Everything works on local server but failing on github action. Keep working. If I have sometimes, I will have a look at your issue.

[agentphoenix]

@driesvints @njoguamos I ended up fixing my issue with Github Actions, though I'm not sure why it fixed it, so maybe one of you knows.

I ended up dumping out the registered sets from the BladeUI icons factory class in my tests. Locally, everything looked as expected. In Github Actions though, it was an empty array. So clearly something was going on where Github Actions was running my tests before that service provider could be called. On a whim, I decided to turn off Laravel's auto-discovery and manually registered the BladeUI icons service provider myself. Everything worked after that.

So the question now is... why? Not sure if there's something about Laravel's auto-discovery and when service providers get called, but this is the second time that removing something from auto-discovery fixed my tests in Github Actions. Welcome to hear thoughts on that if you have any, but either way, thought I'd let you both know that I fixed it and what the fix was.

[driesvints]

@agentphoenix hmm no idea really. I never experienced that myself. Good that you got it fixed though.