Fixed Carousel Arrow SVGs - Added IE11 Compatible Charset for SVGs#6920
Fixed Carousel Arrow SVGs - Added IE11 Compatible Charset for SVGs#6920jridgewell merged 2 commits intoampproject:masterfrom
Conversation
As the title States, added IE11 compatible charset for CSS SVGs. Shorthands will break SVG support
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I have signed the CLA, I look forward to having this reviewed. Thank you! 👍 |
|
CLAs look good, thanks! |
|
/to @camelburrito |
jridgewell
left a comment
There was a problem hiding this comment.
Can you fix the following locations, too?:
- extensions/amp-app-banner/0.1/amp-app-banner.css
- extensions/amp-carousel/amp-carousel.md
- extensions/amp-lightbox-viewer/0.1/amp-lightbox-viewer.css
- extensions/amp-sticky-ad/0.1/amp-sticky-ad.css
- extensions/amp-sticky-ad/1.0/amp-sticky-ad.css
| .amp-carousel-button-prev { | ||
| left: 16px; | ||
| background-image: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" fill="%23fff" viewBox="0 0 18 18"><path d="M15 8.25H5.87l4.19-4.19L9 3 3 9l6 6 1.06-1.06-4.19-4.19H15v-1.5z" /></svg>'); | ||
| background-image: url('data:image/svg+xml;charset=utf-8,<svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" fill="%23fff" viewBox="0 0 18 18"><path d="M15 8.25H5.87l4.19-4.19L9 3 3 9l6 6 1.06-1.06-4.19-4.19H15v-1.5z" /></svg>'); |
There was a problem hiding this comment.
Is it really as simple as this? 😀
|
@jridgewell I suppose that it is that simple! I'm about to have a friend visit from out of town. So I will see if I can make those changes within the next few hours. Is it confirmed that the svg's don't work at the locations you described on IE11? If not, I'd like to test them first, I'll bring my windows external hard drive with me so hopefully I can test it semi-quickly |
|
I can't seem to find a good example to test the other SVGs, the only applicable one seems to be stick ad, which I can't get to show on IE11 :( Either way, I shall make the requested changes! Not Showing in IE11:
|
|
@jridgewell Requested changes have been made :) |
|
@torch2424 - also please make sure nothing is broken on Chome and Safari :) Thanks for fixing this. |
|
@camelburrito In my original post I've proven that these changes do not affect chrome, edge, or Firefox, but only fixes IE11. Also, I do not own any Apple hardware unfortunately, so I was hoping someone could test on safari for me, though I highly doubt it would break anything. (I'm Getting a MacBook pro at the end of the month specifically for this reason) But you are welcome! Glad I could help :) |
|
Tested in iOS 9 and 10, and Safari. |
|
@jridgewell Thank you very much for testing that for me! :) |
…mpproject#6920) * Added IE11 Compatible Charset for SVGs As the title States, added IE11 compatible charset for CSS SVGs. Shorthands will break SVG support * Made All utf8 svg's IE11 compatible
closes #6591
Added IE11 compatible charset for CSS SVGs for amp-carousel. I discovered shorthands will break SVG support in IE 11. And possbily, but NOT in this case, may need to be URL encoded.
Tested using the http://localhost:8000/examples/carousel.amp.max.html, url meant for testing extensions, and tested in IE11, Edge, Google Chrome, and Firefox. See Below for no Safari testing, or if you are interested in testing on Safari for me :)
Supporting Research/Docs:
RFC Docs
CSS Tricks Comment 1
CSS Tricks Comment 2
Testing Browser Screenshots:
IE11
Edge
FireFox
Google Chrome
Safari
Unfortunately, I do not have any apple hardware, it would be much appreciated if someone tested in recent Safari versions for me!