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

document how to setup a sprite loader for use with svg-store #105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lastobelus
Copy link

No description provided.

Copy link
Contributor

@nddery nddery left a comment

Choose a reason for hiding this comment

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

Thanks a lot -- I like the idea of providing more guidance on how to use this feature, but I'd like the example to align more with the premise of Shopify Pipeline.

```

The sprite loader that comes with [webpack-svgstore-plugin]((https://github.com/mrsum/webpack-svgstore-plugin) won't work out of the box, instead given that you have included the above liquid snippet in your theme, this loader will suffice:
Copy link
Contributor

Choose a reason for hiding this comment

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

You have an extra opening parenthesis in the webpack-svgstore-plugin link.

I think we should advocate for writing (and importing) a module instead of putting code directly in a liquid template.

```

The sprite loader that comes with [webpack-svgstore-plugin]((https://github.com/mrsum/webpack-svgstore-plugin) won't work out of the box, instead given that you have included the above liquid snippet in your theme, this loader will suffice:
```
$('[data-svg-loader]').each(function _loadSvg() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like for this example not to use jQuery, as we do not include it ourselves. Maybe using the fetch API ?

Also, I think the example should follow the Dyanmo's ESLint config - so for example, functions shouldn't start with an underscore.

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.

2 participants