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.

Sign up for GitHub

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

Build: regex-based <g> insertion matching svg tag, to be more reliable #259

Merged
merged 1 commit into from
Oct 26, 2017
Merged

Build: regex-based <g> insertion matching svg tag, to be more reliable #259

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions grunt-tasks/svg-transform-add-g.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,8 @@ module.exports = function( grunt ) {
var fileContent = grunt.file.read( 'svg-min/' + svgFile );

// Add <g> to each file
fileContent = fileContent.slice( 0, fileContent.indexOf('viewBox="0 0 24 24">') + 20 ) + // opening SVG tag
'<g>' +
fileContent.slice( fileContent.indexOf('viewBox="0 0 24 24">') + 20, -6 ) + // child elements of SVG
'</g>' +
fileContent.slice( -6 ); // closing SVG tag
fileContent = fileContent.replace( /<svg[^>]*>/i, '$&<g>' ); // add <g> after <svg>
Copy link
Member

Choose a reason for hiding this comment

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

if we run this twice, won't get we <g><g>?

although what @gwwar brought up is 2000% relevant here, that non-RegEx-based parsers are quite important here, for a quick implementation we can find only those <svg things that aren't followed by <g things

const svgWithoutG = [
	// (?:<svg)     - find the opening prefix (non-capture)
	// ([^>]*)      - capture any tag-internal content
	// (?:>(?!<g>)) - followed by a closing `>` which is not also followed by a `<g>`
	/(?:<svg)([^>]*)(?:>(?!<g>))/,
	'<svg\1><g>'
];

const _svgWithoutG = [
	// grab even those with a closing `</g>`
	// then normalize the endings so they all have it
	/(?:(<\/g>)?<\/svg>)/,
	'</g></svg>'
];

Note that what this won't do is handle nesting - if we have nested <g> groups we'll likely mess them up.

Copy link
Contributor Author

@folletto folletto Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

if we run this twice, won't get we <g><g>?

Not a problem: this is always run only on a re-generated file for safety.

I'd rather keep the simpler form, but nice use of lookahead there, and I didn't know there was a way in JS' runtime to do non-capture. TIL!

Copy link
Member

Choose a reason for hiding this comment

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

I meant that when we regenerate the files again the current RegExps will double the group element.

I'd rather keep the simpler form

sure, but neither one is really complicated… the given ones are just a little misleading because of how they leave out the <g> part

fileContent = fileContent.replace( /<\/svg>/i, '</g>$&' ); // add </g> before </svg>

// Save and overwrite the files in svg-min
grunt.file.write( 'svg-min/' + svgFile, fileContent );
Expand Down