Replace Gist links with embedded Gist #820

donocode · 2017-11-16T22:40:28Z

Replaces links with an embedded gist and also injects the stylesheet as a link. Only replaces links that are on their own line and in a comment body of a pr or issue.

Fixes #494

sindresorhus · 2017-11-17T02:45:52Z

src/features/embed-gist-inline.js

+import select from 'select-dom';
+
+const gistRegex = /gist\.github\.com\/\w*?\/\w*/;
+const isGist = link => gistRegex.test(link.href);


Use https://github.com/sindresorhus/refined-github/blob/a8fd502d23084044c282bcdf56bac92666c2238f/src/libs/page-detect.js#L42

This checks the current location. Do you mean to recreate the host or path check?

Never mind. I misread the code.

sindresorhus · 2017-11-17T02:47:18Z

src/features/embed-gist-inline.js

+
+const createGistElement = gistData => {
+  const el = document.createElement('div');
+  const style = `<link rel="stylesheet" href="https://assets-cdn.github.com/assets/gist-embed-3cc724162479db25e452fdf621f2349adef3e742b53552c2a93f82d28156cb96.css" />`;


" /> → ">

sindresorhus · 2017-11-17T02:47:52Z

src/features/embed-gist-inline.js

+const createGistElement = gistData => {
+  const el = document.createElement('div');
+  const style = `<link rel="stylesheet" href="https://assets-cdn.github.com/assets/gist-embed-3cc724162479db25e452fdf621f2349adef3e742b53552c2a93f82d28156cb96.css" />`;
+  el.innerHTML = style + gistData.div;


Do we need to do any sanitizing here? Probably better not to use .innerHTML too.

The data from the gist is sanitised already, are we planning in case a gist response is somehow compromised?

I don't know. I just asked in case anyone could think of a case. I guess it's fine then.

sindresorhus · 2017-11-17T02:48:23Z

src/features/embed-gist-inline.js

+    .all('.js-comment-body p a[href^="https://gist.github.com"]:only-child')
+    .filter(isGist);
+
+  gistLinks.forEach(async link => {


Use a for-of loop

sindresorhus · 2017-11-17T02:48:43Z

src/features/embed-gist-inline.js

+      const gistEl = createGistElement(gistData);
+      const linkParent = link.parentNode;
+      linkParent.parentNode.replaceChild(gistEl, linkParent);
+    } catch (e) {}


e → err

sindresorhus · 2017-11-17T02:49:01Z

src/features/embed-gist-inline.js

+const isGist = link => gistRegex.test(link.href);
+
+const getJsonUrl = href => `${href}.json`.replace(/\.jso?n?\.json$/, '.json');
+const getGistData = href => fetch(getJsonUrl(href)).then(response => response.json());


Use async/await.

More context: #814

sindresorhus · 2017-11-17T02:51:32Z

src/features/embed-gist-inline.js

+const gistRegex = /gist\.github\.com\/\w*?\/\w*/;
+const isGist = link => gistRegex.test(link.href);
+
+const getJsonUrl = href => `${href}.json`.replace(/\.jso?n?\.json$/, '.json');


I don't fully understand what /\.jso?n?\.json$/ is doing. Like, why are we only replacing names that end with .js or .json? Can you show me an example URL? I think this would be clearer regardless /\.(js|json)\.json$/.

It is for if the person links specifically the json or js version, so it won't create a link like https://gist.github.com/user/gist.js.json, it's probably unlikely for someone to be linking it but just in case

Ok, makes sense. I still would like to use /\.(js|json)\.json$/ as it's more readable.

Can you also add a short comment above it to explain what it's doing.

I don't think .js and .json links should be embedded, they are technically not the same content (web page vs embed code)

sindresorhus · 2017-11-26T11:37:32Z

src/features/embed-gist-inline.js

+const getGistData = href => fetch(getJsonUrl(href)).then(response => response.json());
+
+const createGistElement = gistData => {
+	const el = (


No need to use a variable here. Just return it directly.

sindresorhus · 2017-11-26T11:38:39Z

Some more minor nitpick and it's good I think:

sindresorhus · 2017-11-26T11:43:13Z

Why are we injecting the Gist instead of just loading the Gist embed in an iframe?

donocode · 2017-12-01T15:30:39Z

I'll get to this as soon as I can, I haven't forgotten about it!

donocode · 2017-12-06T19:04:39Z

I took a look at an iframe solution, my main worry is the height as is always the case with iframes, it's difficult to set the height of the iframe to be correct for the height of the content. I can deal with this if required but do you think we are gaining from this?

donocode · 2017-12-06T22:22:29Z

It seems the linter does not like await in for of loops although (as far as I'm aware) that is correct usage.

fregante · 2017-12-07T01:54:18Z

src/features/embed-gist-inline.js

@@ -1,30 +1,29 @@
-import { h } from 'dom-chef';


Why was this removed? I think it’s required.

Yes, good catch, my brain was still in iframe refactor mode when I deleted it

fregante · 2017-12-07T01:57:17Z

The linter complains because the loop stops at every await, which means that gists are loaded sequentially rather than in parallel. If you want to use await but load in parallel, extract the loop content into an async function and call it normally from inside the loop, don’t await the call.

fregante · 2017-12-07T02:09:32Z

@sindresorhus is there an iframe version? My understanding is that this is the preferred injection method. Their official embed code is a script tag with document.write

fregante · 2017-12-07T14:52:55Z

@sindresorhus this is the closest I got with an iframe, but it's sadly blocked by GH's CSP

<iframe srcdoc="&lt;script src=&quot;https://gist.github.com/jgierer12/06a8e3913ce4ae608c010b6f47efd257.js&quot;&gt;&lt;/script&gt;">

But you're right, the current solution of injecting the code directly inline isn't working right because of conflicting styles.

Inline	Correct

However the Shadow DOM seems to work correctly and is quite easy to implement it:

link.attachShadow({mode: 'open'}).append(gistEl);

CSP also applies so it still requires the manual fetch

fregante · 2017-12-07T15:20:22Z

This is mergeable for me, but:

some gists may be long or have multiple files
GitHub limits their embeds to 12 lines + scroll:
there's no clear indicator of what file belongs to what gist like for the regular github.com auto-embeds:
there's no loading indicator, the content just pops into place at some point in whatever loading order

Comments were addressed

donocode · 2017-12-11T19:17:54Z

Thanks for the help with this @bfred-it

sindresorhus · 2017-12-12T21:52:37Z

source/features/embed-gist-inline.js

+);
+
+async function embedGist(link) {
+	const response = await fetch(link.href + '.json');


Use template literal here.

sindresorhus · 2017-12-12T21:54:28Z

Looks great now :)

Last thing, can you mention it in the "new feature" highlight in the readme and link to a screenshot showing it in action? https://github.com/sindresorhus/refined-github#new-features

donocode · 2017-12-12T22:27:24Z

Add a cname:

https://gist.github.com/rotemtam/b4474f732674a9fd5884719d8c5bdf3e

donocode · 2017-12-12T22:28:38Z

donocode · 2017-12-12T22:34:01Z

That's all complete now @sindresorhus

sindresorhus · 2017-12-12T22:41:19Z

Yay. Thanks for contributing this and for your patience through the review rounds :)

sindresorhus · 2017-12-12T22:42:51Z

I opened a new issue for the things @bfred-it mentioned: #870

Replace gist links with embedded gist

2a4c254

hkdobrev added the enhancement label Nov 17, 2017

sindresorhus changed the title ~~Replace gist links with embedded gist~~ Replace Gist links with embedded Gist Nov 17, 2017

sindresorhus previously requested changes Nov 17, 2017

View reviewed changes

donocode and others added 4 commits November 17, 2017 11:50

convert spaces to tabs

8bc8950

address simple review comments

869572b

use JSX for gist element

50f2249

Update embed-gist-inline.js

63afc62

sindresorhus reviewed Nov 26, 2017

View reviewed changes

donocode added 2 commits December 6, 2017 18:36

minor review changes

5518228

Merge branch 'master' into embed-gist

4a3c18b

fix xo lint errors

0452cf4

fregante reviewed Dec 7, 2017

View reviewed changes

Fix; Use Shadow DOM version; Exclude embeds

9329687

fregante approved these changes Dec 7, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into donocode-embed-gist

e34d37e

sindresorhus reviewed Dec 12, 2017

View reviewed changes

switch json url to template string

77181d5

add screenshot to new features

cf726ac

sindresorhus merged commit e338b9e into refined-github:master Dec 12, 2017

sindresorhus mentioned this pull request Dec 12, 2017

Possible improvements for the "embed linked gists" feature #870

Closed

Clemens-git76 mentioned this pull request Apr 16, 2024

[Snyk] Upgrade type-fest from 4.7.1 to 4.14.0 Clemens-git76/refined-github#4

Open

Uh oh!

Replace Gist links with embedded Gist #820

Replace Gist links with embedded Gist #820

Uh oh!

Conversation

donocode commented Nov 16, 2017 • edited by sindresorhus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Nov 26, 2017

Uh oh!

sindresorhus commented Nov 26, 2017

Uh oh!

donocode commented Dec 1, 2017

Uh oh!

donocode commented Dec 6, 2017

Uh oh!

donocode commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante commented Dec 7, 2017

Uh oh!

fregante commented Dec 7, 2017

Uh oh!

fregante commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donocode commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Dec 12, 2017

Uh oh!

donocode commented Dec 12, 2017

Uh oh!

donocode commented Dec 12, 2017

Uh oh!

donocode commented Nov 16, 2017 •

edited by sindresorhus

Loading

sindresorhus Nov 21, 2017 •

edited

Loading

sindresorhus Nov 17, 2017 •

edited

Loading

fregante commented Dec 7, 2017 •

edited

Loading

fregante commented Dec 7, 2017 •

edited

Loading