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

Add stripdeclarations option #102

Merged

Conversation

malthejorgensen
Copy link
Contributor

Add the option stripdeclarions to the loader.
If given will tell the loader to strip out any XML declaration, e.g.

<?xml version="1.0" encoding="UTF-8"?>

at the beginning of imported SVGs.

Added because Internet Explorer (tested in Edge 14) cannot handle XML
declarations in CSS data URLs (content: url("data:image/svg...")).

Add the option `stripdeclarions` to the loader.
If given will tell the loader to strip out any XML declaration, e.g.

    <?xml version="1.0" encoding="UTF-8"?>

at the beginning of imported SVGs.

Added because Internet Explorer (tested in Edge 14) cannot handle XML
declarations in CSS data URLs (`content: url("data:image/svg...")`).
@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #102 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #102   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          18     20    +2     
  Branches        4      5    +1     
=====================================
+ Hits           18     20    +2
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b392b01...5b68171. Read the comment docs.

Copy link
Owner

@bhovhannes bhovhannes left a comment

Choose a reason for hiding this comment

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

Internet Explorer (tested in Edge 14) cannot handle XML
declarations in CSS data URLs

Is it documented somewhere? It will be nice to put a link about that in a README.

Otherwise this looks good for me. Thank you!

@malthejorgensen
Copy link
Contributor Author

malthejorgensen commented Jun 23, 2017

Can't find it anywhere, I was hoping caniuse would have it.
And apparently it only happens in Edge (not in IE 11).

Screenshots:
https://www.browserstack.com/screenshots/1ec164d16c5fffe93b88699b21158d1f618fff02

Codepen:
https://codepen.io/malthejorgensen/full/MooKQe/

@bhovhannes
Copy link
Owner

@malthejorgensen, I also found nothing about support, but the screenshot perfectly shows that removing xml declaration indeed fixes icon under Edge.
Maybe it makes sense to always strip xml declaration? Do we need it for other browsers?

@malthejorgensen
Copy link
Contributor Author

malthejorgensen commented Jun 27, 2017

I don't know, you could be right – that it's a better default to always remove the declaration.

I don't really have a good basis to make the decision on – I do like the ability to turn it off though. Most examples (if not all I've seen) do not include the XML declaration, however I don't know if it has any implication on e.g. encoding (which can be specified in the declaration)

@bhovhannes bhovhannes merged commit 64bf99a into bhovhannes:master Jun 28, 2017
@bhovhannes
Copy link
Owner

@malthejorgensen , I am opening a separate issue (#104) about making stripdeclaration the default behavior of the loader.
Your changes are released in version 2.1.0

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