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

GitHub svg media library preview support #954

Merged
merged 3 commits into from
Jan 4, 2018

Conversation

Jinksi
Copy link
Contributor

@Jinksi Jinksi commented Dec 21, 2017

- Summary

In the media library, SVG media loaded from https://raw.githubusercontent.com was unable to be displayed due to github's response content-type set to text/plain rather than image/svg+xml.

Appending a sanitize=true query to the url allows github to return the correct content-type.

- Test plan

Example working SVG preview:
screen shot 2017-12-21 at 3 32 27 pm

- Description for the changelog

Enable SVG preview in media library.

- A picture of a cute animal (not mandatory but encouraged)

emu

@verythorough
Copy link
Contributor

verythorough commented Dec 21, 2017

Deploy preview ready!

Built with commit 70c3090

https://deploy-preview-954--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Dec 21, 2017

Deploy preview ready!

Built with commit 70c3090

https://deploy-preview-954--cms-demo.netlify.com

@Jinksi
Copy link
Contributor Author

Jinksi commented Dec 21, 2017

Info on sanitize=true from this stackoverflow answer.

@erquhart
Copy link
Contributor

This is awesome, thanks @Jinksi!

@tech4him1
Copy link
Contributor

tech4him1 commented Jan 1, 2018

@erquhart Can you double-check this in Safari? According to MDN, URL.searchParams isn't supported in Safari or Edge. I'm guessing we would be able to polyfill it if necessary.

EDIT: CanIUse does show Safari support? https://caniuse.com/#feat=urlsearchparams

return { id: sha, name, size, url: download_url, path };
const url = new URL(download_url);
if (url.pathname.match(/.svg$/)) {
url.searchParams.append('sanitize', true);
Copy link
Contributor

@tech4him1 tech4him1 Jan 1, 2018

Choose a reason for hiding this comment

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

MS Edge does not support searchParams, and it seems to break the entire media library if a single SVG is added.

  1. Upload other images -- media library works normally.
  2. Reload CMS page -- works normally.
  3. Upload SVG -- preview does not load, other image work.
  4. Reload CMS page -- all images missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like Edge will support this in v17: https://caniuse.com/#feat=urlsearchparams. We normally support two versions, though, so it's not practical to just wait until it is released.

Copy link
Contributor

@tech4him1 tech4him1 Jan 1, 2018

Choose a reason for hiding this comment

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

These should be equivalent, and the second supported in all our browsers:

url.searchParams.append('sanitize', true);
url.search += (url.search.slice(1) === '' ? '?' : '&') + 'sanitize=true';

Copy link
Contributor

@tech4him1 tech4him1 Jan 2, 2018

Choose a reason for hiding this comment

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

@Jinksi @erquhart What do you think about using my example code above instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @tech4him1, agreed on the workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erquhart
Copy link
Contributor

erquhart commented Jan 4, 2018

@tech4him1 this was updated, can you merge if you approve?

@tech4him1 tech4him1 merged commit b8c411c into decaporg:master Jan 4, 2018
@tech4him1
Copy link
Contributor

tech4him1 commented Jan 4, 2018

Works great! There is a rendering issue in Microsoft Edge, so I've opened a separate issue for that: #983.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants