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

β›‘ Fix SVGs that are using <g #200

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

pionl
Copy link
Contributor

@pionl pionl commented Sep 19, 2022

πŸ‘‹

I've noticed that some of my SVGs were not properly deferred. This is caused due the strip_tags and content not matching (maybe would be safe to grab the contents between <svg> content - if desired I can make regex that could handle this).

This is screenshot from xdebug while I was debugging the bug.

Snímek obrazovky 2022-09-19 v 20 11 48

I've made the test more strict + added test SVGs with white spaces to make it more safe.

FYI:

I would like to change the "hash" to static value, can I make the PR (using attributes)? I would like to use the SVG in React (this would help me to prevent bugs when someone changes the SVG in source code).

Thanks for the package and have a great day,

@driesvints
Copy link
Member

Hey @pionl. Aren't you looking to defer icons? We have this built in: https://github.com/blade-ui-kit/blade-icons#deferring-icons. We merged that in here: #191

I'm a tad reluctant with merging this as the way I envisioned Blade Icons is to always just work with plain SVG's. This PR would allow Blade to be used within them and I'm not sure that's a path I'd like to head in to.

@pionl
Copy link
Contributor Author

pionl commented Sep 20, 2022

Hi @driesvints

i'm using provided defer option but it is not working with an SVG that uses <g>. This PR only fixes a bug with defer. I've just proposed that it would be safer to change the "replacement" logic to make it more safe for some SVGs.

I'm a tad reluctant with merging this as the way I envisioned Blade Icons is to always just work with plain SVG's. This PR would allow Blade to be used within them and I'm not sure that's a path I'd like to head in to.

I'm sorry I do not understand what you mean with the second parapgrah, are you reluctant to provide the ability to change a hash that defer uses? (not a part of this PR).

This is an example of SVG from adobe XD. As you can see it uses <g everywhere.

When you use defer=1 it will generate SVG with original content and it will push content to bladeicons which is later not used.

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="30" height="30"
     viewBox="0 0 30 30">
    <g>
        <g transform="translate(4.191 5.035)">
            <g transform="translate(0.628 -0.035)">
                <g transform="translate(0)">
                    <path
                        d="M20.069,14.9H16.454V11.288a.776.776,0,0,0-1.552,0V14.9H11.288a.743.743,0,0,0-.776.776.751.751,0,0,0,.776.776H14.9v3.614a.752.752,0,0,0,.776.776.772.772,0,0,0,.776-.776V16.454h3.614a.776.776,0,1,0,0-1.552Z"
                        transform="translate(-5.588 -5.588)" />
                    <path
                        d="M13.466,4.733A8.729,8.729,0,1,1,7.29,7.29a8.674,8.674,0,0,1,6.176-2.557m0-1.358A10.091,10.091,0,1,0,23.556,13.466,10.089,10.089,0,0,0,13.466,3.375Z"
                        transform="translate(-3.375 -3.375)"/>
                </g>
            </g>
        </g>
    </g>
</svg>

@driesvints driesvints merged commit 88eda22 into blade-ui-kit:1.x Sep 21, 2022
@driesvints
Copy link
Member

I see, I thought the Blade tags used in your screenshot were the ones you wrote yourself but it's from the defer option in Blade Icons. I'll merge this for now as I cannot see a really big impact.

I would like to change the "hash" to static value, can I make the PR (using attributes)? I would like to use the SVG in React (this would help me to prevent bugs when someone changes the SVG in source code).

Can you show a specific example of what you want to do? You want to define the hash up front so you can re-use it in React?

@pionl
Copy link
Contributor Author

pionl commented Sep 21, 2022

@driesvints thank you for merging it - sorry that the screenshot was confusing.

Can you show a specific example of what you want to do? You want to define the hash up front so you can re-use it in React?

Yes, that's correct. I want to reuse the icon in React component to prevent "loading" the same icon in react (as another http request or inline). Example:

In the blade as usual:

<x-icon-create hash="create" />

In my react component I would do something like this:

function craeteIcon() {
    return <svg width="30" height="30"><use href="#icon-create"></use>
}

Maybe the has could be generated from the "package" and the icon name but this would possibly be a BC.

Thank you for your time

@driesvints
Copy link
Member

Can't we pass a custom hash to the defer attribute itself?

<x-icon-camera defer="my-custom-hash" />

@pionl pionl deleted the fix-grouped-svg branch September 22, 2022 12:19
pionl added a commit to pionl/blade-icons that referenced this pull request Sep 22, 2022
- Ideal when you want to reuse deferred icons in your React/Vue components
- Related PR discussion blade-ui-kit#200
@pionl
Copy link
Contributor Author

pionl commented Sep 22, 2022

I do like the idea πŸ‘ Done #201

pionl added a commit to pionl/blade-icons that referenced this pull request Sep 22, 2022
- Ideal when you want to reuse deferred icons in your React/Vue components
- Related PR discussion blade-ui-kit#200
pionl added a commit to pionl/blade-icons that referenced this pull request Sep 22, 2022
- Ideal when you want to reuse deferred icons in your React/Vue components
- Related PR discussion blade-ui-kit#200
driesvints added a commit that referenced this pull request Sep 28, 2022
* Add ability to provide own defer hash

- Ideal when you want to reuse deferred icons in your React/Vue components
- Related PR discussion #200

* Update docs

* Update Svg.php

* Update README.md

Co-authored-by: Dries Vints <[email protected]>
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.

2 participants