Skip to content

Conversation

@davidwebca
Copy link

Gradients get grabbed by the regex right now because of the too general id="(\S+)" regex. Gradients need ids so they can get referenced later and when you have gradients in your "defs", it gets grabbed as well. At first, I wanted to resolve this problem by grabbing only the IDs that aren't between defs with the regex, but that gets complicated, so I went with the more simple strip_tags route by keeping only symbol and g which are the two most popular ways to build svg icon sprites.

Here's an example svg sprite that would cause to have an empty choice because of the gradient in defs :

<svg class="svg-icons-definitions" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <defs>
        <linearGradient x1="33.236%" y1="-36.335%" x2="66.808%" y2="-36.179%" id="0a"><stop stop-color="#EDF0F7" offset="0%"></stop><stop stop-color="#FAFAFA" offset="100%"></stop></linearGradient><linearGradient x1="44.849%" y1="-105.823%" x2="55.16%" y2="-105.541%" id="0b"><stop stop-color="#7045FF" offset="0%"></stop><stop stop-color="#36BFEF" offset="100%"></stop></linearGradient>
    </defs>
    <symbol id="icon-base" viewBox="0 0 180 111">
        <title>base</title><g fill-rule="nonzero" fill="none"><g fill="url(#0a)"><path d="M0 52.7L89.8.6 180 52.9l-89.7 52z"></path><g transform="translate(0 .6)"><path d="M0 52.1L89.8 0 180 52.3l-89.7 52z"></path><path d="M90.3 104.3L0 52.1 89.8 0 180 52.3z"></path></g></g><path d="M180 .2v5.5L90.3 57.8 0 5.6.1 0l90.2 52.2z" transform="translate(0 52.7)" fill="url(#0b)"></path></g>
    </symbol>
</svg>

Gradients get grabbed by the regex right now because of the too general ```id="(\S+)"``` regex. Gradients need ids so they can get referenced later and when you have gradients in your "defs", it gets grabbed as well. At first, I wanted to resolve this problem by grabbing only the IDs that aren't between <defs></defs> with the regex, but that gets complicated, so I went with the more simple strip_tags() route by keeping only <symbols> and <g> which are the two most popular ways to build svg icon sprites.

Here's an example svg sprite that would cause to have an empty choice because of the gradient in <defs> : ```
<svg class="svg-icons-definitions" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
		<defs>
				<linearGradient x1="33.236%" y1="-36.335%" x2="66.808%" y2="-36.179%" id="0a"><stop stop-color="#EDF0F7" offset="0%"></stop><stop stop-color="#FAFAFA" offset="100%"></stop></linearGradient><linearGradient x1="44.849%" y1="-105.823%" x2="55.16%" y2="-105.541%" id="0b"><stop stop-color="#7045FF" offset="0%"></stop><stop stop-color="#36BFEF" offset="100%"></stop></linearGradient>
		</defs>
		<symbol id="icon-base" viewBox="0 0 180 111">
			<title>base</title><g fill-rule="nonzero" fill="none"><g fill="url(#0a)"><path d="M0 52.7L89.8.6 180 52.9l-89.7 52z"></path><g transform="translate(0 .6)"><path d="M0 52.1L89.8 0 180 52.3l-89.7 52z"></path><path d="M90.3 104.3L0 52.1 89.8 0 180 52.3z"></path></g></g><path d="M180 .2v5.5L90.3 57.8 0 5.6.1 0l90.2 52.2z" transform="translate(0 52.7)" fill="url(#0b)"></path></g>
		</symbol>
</svg>
```
@Rahe
Copy link
Member

Rahe commented Apr 12, 2018

Hello,

Thank you for the PR, this is really interesting and can even boost the performances to not preg_Replace the whole text, elegant solution by the way :)
Is there any other SVG tags that can be used for icons ? (symbol and g as you describe them)

Nicolas,

@davidwebca
Copy link
Author

Hi Rahe,

I don't think there are other tags available or commonly used, but I guess that giving users a filter to allow different html tags to pass to strip_tags would prevent edge cases to arise. Like :

$tags = '<symbol><g>';
$tags = apply_filters( 'acf_svg_icon_tags',  $tags);
preg_match_all( '#id="(\S+)"#', strip_tags($contents, $tags), $svg );

What do you think?

@Rahe
Copy link
Member

Rahe commented Apr 13, 2018

Hello David,

This is a great idea, for the filter name I think acf_svg_icon_svg_parse_tags could be more understandable :)

@davidwebca
Copy link
Author

Here you go! I've structured this in an external function to keep it clean and to document what it does properly.

@MaximeCulea
Copy link
Contributor

Hi @david-treblig,
Couldn't merge, but backported into master with credits.
Thx, I close this PR.

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.

3 participants