blade-ui-kit / blade-icons

A package to easily make use of SVG icons in your Laravel Blade views.
https://blade-ui-kit.com/blade-icons
MIT License
2.21k stars 148 forks source link

Apply SVG file filter for Generator by Default #209

Closed mallardduck closed 1 year ago

mallardduck commented 1 year ago

This allows icon package devs to define a filter closure applied to the list of discovered files. If the upstream icon package starts to included non-SVG files in Icon set folders this can filter those out.

Background

Coming back to some of my icon packages I found some errors when generating them:

~/MyGitProjects/blade-lucide-icons
± |main ↑1 U:9 ?:48 ✗| → ./vendor/bin/blade-icons-generate
Starting to generate icons...

Warning: DOMDocument::load(): Start tag expected, '<' not found in /Users/danpock/MyGitProjects/blade-lucide-icons/resources/svg/accessibility.json, line: 1 in /Users/danpock/MyGitProjects/blade-lucide-icons/config/generation.php on line 5

Fatal error: Uncaught Error: Call to a member function removeAttribute() on null in /Users/danpock/MyGitProjects/blade-lucide-icons/config/generation.php:7

I've determined this is caused by my "upstream icon" package adding JSON files into the icons directory. As such I now need a way to ensure the generator only interacts with the correct files.

This PR modifies the files array to ensure only .svg extension files are included in the list passed to the generator.

mallardduck commented 1 year ago

@driesvints - On a semi-related note, I'd also love to send a PR that will add a config object for the generator. This way we improve the DX for our end users via discoverability of config options. Obviously this would also be non-BC breaking change so that existing array based configs will still work.

driesvints commented 1 year ago

Heya, thanks for this. Just wondering if we can't just filter for .svg by default and not add the filter?

On a semi-related note, I'd also love to send a PR that will add a config object for the generator. This way we improve the DX for our end users via discoverability of config options. Obviously this would also be non-BC breaking change so that existing array based configs will still work.

If you send in a PR we can have a look. Would depend on how complex it would be.

mallardduck commented 1 year ago

Hiya, so I had the thought to just filter for only SVGs too. For some reason I decided that other uses might want a different behavior. But TBH in hindsight I can't really point at a specific reason they would need that.

driesvints commented 1 year ago

This library really is only for SVG icons so I can't fathom why anyone would use something else.

mallardduck commented 1 year ago

This library really is only for SVG icons so I can't fathom why anyone would use something else.

For sure, totally makes sense - just been so long since I've done development lol. 🧠 fart. Updated the branch and PR now.

driesvints commented 1 year ago

@mallardduck this lgtm. Can we also add a test maybe?

mallardduck commented 1 year ago

@driesvints - Was on a mini-vacation, just found some time to add tests to cover the generator class. Got it up to 100% coverage on that file. These tests should add some assurance for when I work on the config class idea too!

driesvints commented 1 year ago

Looks great. Thanks for your work @mallardduck!