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

Apply SVG file filter for Generator by Default #209

Merged
merged 6 commits into from
Feb 15, 2023

Conversation

mallardduck
Copy link
Contributor

@mallardduck mallardduck commented Feb 7, 2023

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 and others added 2 commits February 7, 2023 11:58
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.
@mallardduck
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@mallardduck mallardduck changed the title Add ability for Generator to filter files Apply SVG file filter for Generator by Default Feb 9, 2023
@mallardduck
Copy link
Contributor Author

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
Copy link
Member

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

@mallardduck
Copy link
Contributor Author

@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 driesvints merged commit b2a80ff into blade-ui-kit:1.x Feb 15, 2023
@driesvints
Copy link
Member

Looks great. Thanks for your work @mallardduck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants