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

A big thank you and some suggestions (DEFAULT_CONFIG, SVG_PROPS and SVG_STYLE) #22

Closed
Scriptura opened this issue Nov 6, 2022 · 5 comments · Fixed by #23
Closed

A big thank you and some suggestions (DEFAULT_CONFIG, SVG_PROPS and SVG_STYLE) #22

Scriptura opened this issue Nov 6, 2022 · 5 comments · Fixed by #23
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@Scriptura
Copy link

Scriptura commented Nov 6, 2022

Good Morning Mr Atanasov,

First of all, thank you for taking the trouble to follow up on the work of Hugo/Kitty Giraudel and for writing this excellent plugin. I first wanted to take the svg-sprite plugin (suggested by Google itself in one of its Material icons pages for developers), but the problem with this plugin is that it is too heavy, it makes coffee! With this plugin we spend our time configuring the options we want to deactivate. And above all it is difficult to determine the final path of the sprite, in the end you have to copy/paste with node to get what you want.

Your plugin does what we ask of it: an SVG sprite!

In short, I come to you, here are some suggestions from me that you will find, I hope, benevolent:

  1. First of all, a detail: on the npm page the link to the atanas.info page is broken (https://atanas.info/projects/svg-symbol-sprite.html -> https://atanas.info/portfolio/open-source/svg-symbol-sprite). It's a minor detail but almost made me miss your excellent plugin.

  2. If you have configured your environment for ES6 modules, you get an error indicating a path problem for the SVGO configuration file. I had trouble understanding where the error came from and I solved it by directly installing the SVGO module for my icons: being configured in "type": "module" I had to change my file to *.cjs . Incidentally, I think that the optimization of the icons must be done upstream and is to be left to the responsibility of the developer, it would be necessary to be able to deactivate this option.

  3. We should also be able to customize the SVG_PROPS and SVG_STYLE variables.
    3.1. At the time of HTM5 now well installed, the declaration of a namespace for the SVG is implicit and therefore useless.
    3.2. the same for styles, unless the sprite is injected inline, I don't see the point. And on the other hand there are several schools on accessibility: some developers might prefer the sr-only method for example.

There you go, I hope you won't take offense at these few remarks from an amateur practicing as a dilettante.

Best regards.

@scriptex
Copy link
Owner

scriptex commented Nov 7, 2022

Hi, @Scriptura, thank you for your kind words and the time you took to summarize your feature requests.
I have released a new version (1.2.2) which fixes the wrong URL to the documentation on my website, thanks for noticing this :)
Regarding the rest of the suggestions, I am planning on adding them as all of them make sense but I don't have the time to do this right now.
Have you considered opening a PR for this?
If not, it might take a while...

@scriptex scriptex self-assigned this Nov 7, 2022
@scriptex scriptex added enhancement New feature or request good first issue Good for newcomers labels Nov 7, 2022
@Scriptura
Copy link
Author

Scriptura commented Nov 7, 2022

Ideally I would do something like this (but I'm too amateur to test...), change only for styles and namespaces, no deactivation of SVGO:

//const SVG_PROPS = 'xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" aria-hidden="true"';
//const SVG_STYLE = 'style="width: 0; height: 0; position: absolute;"';
const DEFAULT_CONFIG = join(__dirname, '..', 'config', 'svgo.config.js');

/* ... */

cli.option('-i, --input [input]', 'Specifies input dir (current dir by default)', '.')
	.option('-o, --output [output]', 'Specifies output file ("./sprite.svg" by default)', 'sprite.svg')
	.option('-v, --viewbox [viewbox]', 'Specifies viewBox attribute (parsed by default)', '')
	.option('-p, --prefix [prefix]', 'Specifies prefix for id attribute ("icon-" by default)', 'icon-')
	.option('-c, --config [config]', 'Absolute path to the SVGO config file', './config/svgo.config.js')
	.option('-n, --namespace [namespace]', 'Namespaces and aria attribute for the SVG sprite', 'xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" aria-hidden="true"')
	.option('-s, --style [style]', 'Inline styles for the SVG sprite', 'style="width: 0; height: 0; position: absolute;"')
	.parse(process.argv);

const { input: INPUT, output: OUTPUT, viewbox: VIEWBOX, prefix: PREFIX, config: CONFIG, namespace: NAMESPACE, style: STYLE } = cli.opts();

/* ... */

const getSpriteContent = (contents: string[]) => `<${['svg', NAMESPACE, STYLE].join(' ')}>${contents.join('')}</svg>`;

Which would allow us to do this in the package.json:
"svg-symbol-sprite -n '' -s ''"

Not only am I not able to make a PR, but I can't test these modifications... to be seen...

@scriptex
Copy link
Owner

scriptex commented Nov 7, 2022

I see, okay!
Let me find some time for this and I will implement it.

@scriptex scriptex linked a pull request Nov 10, 2022 that will close this issue
@scriptex
Copy link
Owner

Hi again, @Scriptura.

I've published a new version on NPM (1.3.0) and on Github which tackles everything you mentioned in this issue.

  1. You can use -p or --props to set custom SVG props or just pass -p "" to remove any SVG props.
  2. You can use -s or --style to set custom inline style for the SVG or just pass -s "" to remove any inline style.
  3. You can also disable the SVGO files optimization using the -c or --config flag by passing false to it: -c false.

Thanks for your really important input and I hope you like how the package works now :)

@Scriptura
Copy link
Author

Hello,

Thank you for making all these changes.

If you allow me, a note of optimization:
If we type the command -a '' -s '' we get <svg style="">, so white spaces and an empty attribute for styles. That's why I came up with something like this:

<${['svg', NAMESPACE, STYLE].join(' ')}>

which we could now translate, following your naming conventions, and taking up all of line 51, by:

const getSpriteContent = (contents: string[]) => `<${['svg', ATTRS, STYLE].join(' ').replace('style=""', '')}">${contents.join('')}</svg>`;

Greetings from France to Bulgaria!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants