feat: implement a shared Glitch shortcode #68
Conversation
shortcodes/Glitch.js
Outdated
| */ | ||
|
|
||
| const {html} = require('common-tags'); | ||
| const {escape, stringify} = require('querystring'); |
There was a problem hiding this comment.
Please use the Node.js native URL module instead.
shortcodes/Glitch.js
Outdated
| if (!id) { | ||
| return; | ||
| } | ||
| const url = 'https://glitch.com/embed/#!/embed/' + escape(id); |
There was a problem hiding this comment.
Use new URL instead.
shortcodes/Glitch.js
Outdated
| const defaultAllow = [ | ||
| 'camera', | ||
| 'clipboard-read', | ||
| 'clipboard-write', | ||
| 'encrypted-media', | ||
| 'geolocation', | ||
| 'microphone', | ||
| 'midi', | ||
| ]; |
There was a problem hiding this comment.
move to top of file as configuration constant, like const DEFAULT_ALLOW.
shortcodes/Glitch.js
Outdated
| ).join('; '); | ||
| } | ||
|
|
||
| const src = `${url}?${stringify(queryParams)}`; |
There was a problem hiding this comment.
Use URLSearchParams instead.
shortcodes/Glitch.js
Outdated
| if (path) { | ||
| queryParams.path = path; | ||
| } | ||
| if (highlights) { | ||
| queryParams.highlights = highlights; | ||
| } | ||
| if (typeof previewSize === 'number') { | ||
| queryParams.previewSize = previewSize; | ||
| } |
There was a problem hiding this comment.
Have another look at the d.c.c Glitch implementation. It already uses URLSearchParams: https://github.com/GoogleChrome/developer.chrome.com/blob/main/site/_shortcodes/Glitch.js
There was a problem hiding this comment.
Waiting for @mamieorine to test with web.dev and d.c.c and report back.
|
@matthiasrohmer I have tested a shared Glitch shortcode with web.dev and d.c.c, it works fine with both site. The results of testing a shortcode with both web.dev and d.c.c. are shown below: |
shortcodes/Glitch.js
Outdated
| let allow = Array.from(new Set([...DEFAULT_ALLOW])).join('; '); | ||
| if (glitchProps.allow) { | ||
| allow = Array.from( | ||
| new Set([...DEFAULT_ALLOW, ...expandAllowSource(glitchProps.allow)]) | ||
| ).join('; '); | ||
| } |
There was a problem hiding this comment.
Just seeing this now, something weird is happening here: DEFAULT_ALLOW is always the same, and it's an array. So:
DEFAULT_ALLOW;
[...DEFAULT_ALLOW];
Array.from(new Set([...DEFAULT_ALLOW]))are all the same. The spread into a new array only makes sense in L89.
There was a problem hiding this comment.
I have fixed it. I have no idea why I wrote that weird code 😅
Resolves #64
The Glitch shortcode on both d.d.c and web.dev have the same design and code structure. So, this shortcode can be moved into webdev-infra so it will be a shared shortcode which can be used by both d.c.c and web.dev.
Changes in this pull request:
Usages:
{% Glitch 'tabindex-zero' %}