-
-
Notifications
You must be signed in to change notification settings - Fork 148
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 a general generator for icon package maintainers #144
Add a general generator for icon package maintainers #144
Conversation
I feel like this would be kind of cool to have as a single-command application in the packages, so you'd basically just have an executable #!/usr/bin/env php
<?php
require __DIR__.'/vendor/autoload.php';
use BladeUI\Icons\Console\GenerateCommand;
GenerateCommand::create()
->fromNpmPackage('@fortawesome/fontawesome-free') // Optionally specify an `npm` package to load from
->fromSourceSvgDirectory('/svgs') // Specify a directory, if an npm package isn't set, this can be anywhere
->withIconSets([
'regular' => 'far-',
'brands' => 'fab-',
'solid' => 'fas-',
])
->withSvgNormalisation(function (string $tempFilepath, string $iconSet) {
// Any required SVG manipulation, called after copying the files (equivalent to normalizeSvgFile)
})
->run(); This makes the process even cleaner, and doesn't require extending the class unless the package requires something additional. It also means there would only be a single file (the |
@owenvoke - I'm pretty novice in the symfony space still -since I usually stick with laravel- so I'm not sure if what you're showing is possible. But from my limited playing around with it I know it's not impossible. And TBH that seems like a pretty decent end user API - one that I could even potentially see supporting both the "manually downloaded icon source" and the "fetches this from npm icon source" methods. Seems like ultimately the main "issue" i'm thinking of with your example is just really semantics of naming - since I think it needs to be a The one area I'm wondering about expanding that neither of our implementations cover yet is non-uniform directory structures. As it seems some icon packs have the "default" set at |
Wow! I love the changes, looks great. 👏🏻 |
Alright - so the current state of things seems to be pretty optimal for the icon packages I sent PRs to. I'm sure @driesvints may have some feedback directly, but also curious if we should seek feedback from other authors? One thing I think we could improve potentially is around the variation found in different Icon packages source projects. I've done my best to encapsulate this logic in the So in the case of Heroicons, multiple source folders/sets can render down to a single IconSet prefixes (and output folder) where each SVG file also has a filename prefix. And in my case with boxicons I can render them out to multiple sets with unique IconSet prefixes. In theory this same functionality would be where someone could configure things to look for the "main" set in a "root" folder. And then secondary sets in sub folders. |
Random request, could we add a |
@owenvoke - yeah, no problem adding that at all. As you guessed, your fontawesome package lost the Adobe Icon when I ran it with that change. |
Any changes or help needed on this? 👍🏻 🙂 |
Sorry, haven't found time to look into this yet. I'll try to do this soon but no promises. |
@driesvints - awesome. just to confirm, I can start working on some of those adjustments discussed right? |
@mallardduck yes! Really appreciate that 🙂 |
this allows package maintainers to simply make the config file that builds the command.
@driesvints the bin script changes were surprisingly simple to get working. At least with the implementation that doesn't take any user input. The current bin script is only meant to be run from the packages root - not a laravel app consuming the icon pack. It does seem to work really well though. Should be ready for final review! |
@mallardduck awesome. I'm a bit out of time this week. Not sure when exactly I'll get back to this but hope so soon. Thanks for your work! |
Okay, I sort of worked the entire day on this PR and had way too much fun refactoring it to what it is now 😅 Basically I stripped it down a lot. All functionality still remains but is much simpler now. Took down the Pr from 330 lines to 99 lines of code. Take a look also at the other commit I made for Blade Heroicons: blade-ui-kit/blade-heroicons#19 @owenvoke @mallardduck Bit too tired now to type a lengthy explanation. Just look over the PR and let me know if that all looks good or not 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the few comments and changes I've made things seem to work as advertised.
Once I updated my config to the more simple array based config sets I get the same output.
See here for my packages implementation: https://github.com/mallardduck/blade-boxicons/pull/1/files
Overall looks good if these changes I made work for you guys.
@owenvoke - I've updated the PR on your repo as well and tested it. All looks good.
So basically the |
71039c5
to
cafad03
Compare
@mallardduck thanks for the explanation. I copied the same use as the envoy binary now and it works as expected. |
Thanks for this @mallardduck & @owenvoke! I'll write docs and release in a few. |
This PR adds the ability for
blade-icons
consumer libraries to easily create a generation script. TheGenerateCommandBuilder
accomplishes this by providing a fluent API to build aSingleCommandApplication
that functions in 3 phases:Using a builder style pattern seems to be a better fit for both end-users and providing a good general purpose solution. At the current state I think it should be much more functional for most use cases compared to the first pass attempt.
However, this is still a rather basic script and it could have some short comings for some Icon packs. Namely if the icon pack stores the "default" (or regular) set in the root directory and has variant sets in sub directories.
For those cases it may make more sense to introduce a
IconSetParameters
DTO of sorts that would encapsulate the icon sets state and location. More input from other authors, or direct investigation into other icon packs may be needed.Here's what my boxicons package would use to make the generator work:
Example Implementations:
My Boxicons Package: https://github.com/mallardduck/blade-boxicons/pull/1/files
@owenvoke 's FontAwesome package: owenvoke/blade-fontawesome#25
@driesvints Heroicons package: blade-ui-kit/blade-heroicons#19