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 build script and source files #19

Closed
wants to merge 4 commits into from

Conversation

climech
Copy link
Collaborator

@climech climech commented Feb 2, 2021

(This request depends on the other one fixing color-bleeding issues. Check it out first before reading this! Big thanks for making this awesome icon set, BTW!)

Add build script and source files

This adds scripts/build.js and a src/ directory containing the "base" unmasked icons from which the builds are generated. The flags/ directory becomes the location for the default build, preserving backward compatibility.

Building/contributing instructions were added to README.md.

Purpose

  1. Simplify the development/design process by automating the repetitive task of applying masks
  2. Make it possible for users to make customized builds

Features

At the moment the script can apply a circular/rounded rect mask, allowing the radius to be customized. It uses a <circle> as the mask shape when CORNER_RADIUS is set to 50% (default), otherwise <rect> is used.

The script could be extended in the future, e.g. by making it possible to use a custom palette to replace the default one.

Dependencies

The script adds 2 devDependencies to package.json:

  • svgo
  • xmlbuilder2

Usage

Default build (all flags):

$ npm install
$ npm run build

Selected flags only:

$ npm run build gb us ca

Customized build with a 25% corner radius:

$ BUILD_DIR=rounded/ CORNER_RADIUS=25% npm run build

Recognized environment variables:

Variable Default value
SRC_DIR src/
BUILD_DIR build/
CORNER_RADIUS 50%

This adds `scripts/build.js` and a `src/` directory containing the "base"
unmasked icons from which the builds are generated. The `flags/` directory
becomes the location for the default build, preserving backward compatibility.
README.md Outdated Show resolved Hide resolved

Then run `svgo` to optimize the SVG files:
The files will appear under `build/`. Check if everything looks good, then move the files into `flags/`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to have the default build command output to flags/ directly? Since this is a git directory, it's not like overwriting the files would cause any irreversible damage. Unless you think this is useful for comparing the original and the edited built files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I used it that way myself, to compare the file sizes against the default build, but maybe an extra "staging area" like that is redundant. I thought perhaps this could be useful to users who haven't decided yet which variant they want to use, so they could compare the builds side by side in their file manager. I'd like to see what others think before changing it!

const xmlb = require("xmlbuilder2");
const SVGO = require("svgo");

const svgoOptions = {
Copy link
Collaborator

@waldyrious waldyrious Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions:

  1. How can we make sure this list will remain in sync with svgo.yml? Perhaps a comment on both locations would help in that regard.
  2. It's a valid approach to document all options explicitly, rather than only those set differently than svgo's defaults. However, that loses us that distinction, which I feel is valuable to illustrate what this project has deliberately chosen to configure.
    • Maybe add comments next to the non-default lines specifying that they differ from the defaults?
    • Alternatively, we could simply use the same approach as svgo.yml, and leave out the options matching svgo's defaults, adding a comment above this object linking to https://github.com/svg/svgo#what-it-can-do, where the defaults are listed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SVGO just got a new maintainer who's been doing some intensive work recently, and v2 was just released. It includes a change that could help us out here—we could just import the config inside the script and avoid repetition. The YAML format is deprecated in the new version, though, replaced with a svgo.config.js exporting a config object.

The new version also adds extendDefaultPlugins as part of the new config scheme, which addresses the 2nd point too! No need for the long list of plugins.

This will require a bump to SVGO v2, I'll see if it doesn't break anything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing news! Thanks for sharing. I would expect there not to be much breakage, and whatever differences may exist to be fixed via configuration. Of course, if it does turn out to be cumbersome to upgrade, we could certainly leave that as a future improvement for another PR.

@waldyrious
Copy link
Collaborator

@climech I love this work, awesome job! The square flags look amazing and I'm actually wondering if we shouldn't consider documenting in the README how they can be used directly. Maybe even have a couple more gallery pages displaying a square and a rounded-corners variants of the flags. But that can remain for a different PR. 🙂

Co-authored-by: Waldir Pimenta <waldyrious@gmail.com>
Base automatically changed from master to main February 25, 2021 11:00
@HatScripts HatScripts closed this Feb 26, 2021
@HatScripts HatScripts deleted the branch HatScripts:main February 26, 2021 15:29
@waldyrious
Copy link
Collaborator

waldyrious commented Feb 27, 2021

@HatScripts I believe the branch renaming retargeted all open PRs (which would be just this one) from master to main, and the deletion of main as discussed in #20 then closed all open PRs targeting that branch (again, just this one was affected). Sorry for suggesting that course of action myself! 😳

I suppose it might be possible to manually retarget the PR towards gh-pages, either by you or by @climech (or both). If not, the PR would have to be recreated.

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

Successfully merging this pull request may close these issues.

None yet

3 participants