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 a general generator for icon package maintainers #144

Merged
merged 31 commits into from Jun 25, 2021

Conversation

mallardduck
Copy link
Contributor

@mallardduck mallardduck commented May 11, 2021

This PR adds the ability for blade-icons consumer libraries to easily create a generation script. The GenerateCommandBuilder accomplishes this by providing a fluent API to build a SingleCommandApplication that functions in 3 phases:

  1. Generate temp files for normalization from source files,
  2. Normalize the SVGs in the temp directory, (optional, only used when closure provided)
  3. Relocate the normalized SVGs to their destination path.

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:

GenerateCommandBuilder::create('blade-boxicons')
    ->fromNpmPackage('boxicons') // Optionally specify an `npm` package to load from
    ->fromSourceSvgDirectory('/svg') // Specify a directory, if an npm package isn't set, this can be anywhere
    ->withIconSets([
        IconSetConfig::create('regular')
            ->setInputFilePrefix('bx-'),
        IconSetConfig::create('logos')
            ->setInputFilePrefix('bxl-'),
        IconSetConfig::create('solid')
            ->setInputFilePrefix('bxs-'),
    ])
    ->withSvgNormalisation(function (string $tempFilepath, string $iconSet) {
        $doc = new DOMDocument();
        $doc->load($tempFilepath);
        /**
         * @var DOMElement $svgElement
         */
        $svgElement = $doc->getElementsByTagName('svg')[0];
        $svgElement->removeAttribute('width');
        $svgElement->removeAttribute('height');
        $svgElement->removeAttribute('viewBox');
        $svgElement->setAttribute('fill', 'currentColor');
        $svgElement->setAttribute('stroke', 'none');
        $svgElement->setAttribute('viewBox', '0 0 24 24');
        $doc->save($tempFilepath);

        $svgLine = trim(file($tempFilepath)[1]);
        file_put_contents($tempFilepath, $svgLine);
    })
    ->run();

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

@mallardduck mallardduck changed the title Add a general generator for icon package maintainers [RFC] Add a general generator for icon package maintainers May 11, 2021
@owenvoke
Copy link
Contributor

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 generate file which contains:

#!/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 generate binary).

@mallardduck
Copy link
Contributor Author

@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 *CommandBuilder class of sorts - potentially. Really appreciate that feedback on this PR, I'll start working towards something similar to the example you provided.


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 svg/*.svg and then other sets at svg/{SET}/*.svg. So that makes me consider promoting the icon set from a key/value pair to a config class. 🤔

@owenvoke
Copy link
Contributor

Wow! I love the changes, looks great. 👏🏻

@mallardduck
Copy link
Contributor Author

mallardduck commented May 12, 2021

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 IconSetConfig.

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.

src/Console/IconSetConfig.php Outdated Show resolved Hide resolved
src/Console/GenerateCommandBuilder.php Outdated Show resolved Hide resolved
@owenvoke
Copy link
Contributor

Random request, could we add a clearDestinationDirectory() method, for my Font Awesome package they seem to remove icons due to copyright sometimes and it might be useful. Although, to be honest, I'm not keen on removing icons as it might be a breaking change to someone's site. 🤷🏻

@mallardduck
Copy link
Contributor Author

@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.

@mallardduck mallardduck changed the title [RFC] Add a general generator for icon package maintainers Add a general generator for icon package maintainers May 13, 2021
@owenvoke
Copy link
Contributor

Any changes or help needed on this? 👍🏻 🙂

@driesvints
Copy link
Member

Sorry, haven't found time to look into this yet. I'll try to do this soon but no promises.

@mallardduck
Copy link
Contributor Author

@driesvints - awesome. just to confirm, I can start working on some of those adjustments discussed right?

@driesvints
Copy link
Member

driesvints commented Jun 3, 2021

@mallardduck yes! Really appreciate that 🙂

this allows package maintainers to simply make the config file that builds the command.
@mallardduck
Copy link
Contributor Author

@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!

@driesvints
Copy link
Member

driesvints commented Jun 4, 2021

@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!

bin/blade-icons-generate Outdated Show resolved Hide resolved
@driesvints
Copy link
Member

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 😅

Copy link
Contributor Author

@mallardduck mallardduck left a 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.

bin/blade-icons-generate Outdated Show resolved Hide resolved
.gitattributes Show resolved Hide resolved
bin/blade-icons-generate Outdated Show resolved Hide resolved
@mallardduck
Copy link
Contributor Author

@owenvoke - I've updated the PR on your repo as well and tested it. All looks good.
The litmus test for success here (IMHO) was:

  1. deleting all of resources/svg/,
  2. checking that change in,
  3. running the builder, and
  4. checking those changes-in to ensure no files were actually "changed" in the end.

So basically the safe flag but a little more manual and direct to be more through.
Doing that on both of our repos after updating the config show'd zero changes.

.gitattributes Outdated Show resolved Hide resolved
@driesvints
Copy link
Member

@mallardduck thanks for the explanation. I copied the same use as the envoy binary now and it works as expected.

@driesvints driesvints merged commit a552d40 into blade-ui-kit:1.x Jun 25, 2021
@driesvints
Copy link
Member

Thanks for this @mallardduck & @owenvoke! I'll write docs and release in a few.

@driesvints
Copy link
Member

https://github.com/blade-ui-kit/blade-icons#generating-icons

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.

None yet

3 participants