google / typograms

Apache License 2.0
1.31k stars 18 forks source link

Replace <pre> element contents instead of inserting after <script> #4

Open ConcernedHobbit opened 1 year ago

ConcernedHobbit commented 1 year ago

Issue based on this HackerNews comment by @chris-morgan: https://news.ycombinator.com/item?id=37048556

<script type="text/typogram">

This should use <pre> instead, e.g. <pre class="typogram">. It’s content, not scripting, and if the JavaScript isn’t run (for whatever reason—JS is less reliable than people often think, especially third-party JS, even on environments that don’t try to block it) you’d like the diagram to still be visible in some form.

My suggestion would be to use a custom data attributes (i.e. data-typogram) instead, but it might just be personal preference.

Retaining the <pre> would also be a great improvement for selecting text—the current arrangement of “place single-character <text> elements” is almost useless for copy and paste (losing spaces and line breaks), which is the main reason I can imagine wanting such a thing.

ConcernedHobbit commented 1 year ago

It would also be more semantically correct. Crawlers would be able to pick up on the preformatted text and use it for e.g. better search results.

westonruter commented 1 year ago

Another benefit of this is it would eliminate layout shift when the JS loads, thus improving the CLS metric.

It would also work in RSS feeds.

lucaskatayama commented 1 year ago

@ConcernedHobbit Did that by downloading the https://google.github.io/typograms/typograms.js And changing the last lines:

  1. The query selector
  2. The insertBefore to replaceWith

From:

document.addEventListener("DOMContentLoaded",(function(){for(const e of document.querySelectorAll("script[type='text/typogram']")){if(e.hasAttribute("disabled"))continue;const n=e.innerText,r=Number(e.getAttribute("zoom")||.3),s=e.hasAttribute("grid"),i=t(n,r,s);e.parentNode.insertBefore(i,e.nextSibling)}}))}();

To:

 document.addEventListener("DOMContentLoaded", (function () { for (const e of document.querySelectorAll("pre[data-typogram]")) { if (e.hasAttribute("disabled")) continue; const n = e.innerText, r = Number(e.getAttribute("zoom") || .3), s = e.hasAttribute("grid"), i = t(n, r, s); e.replaceWith(i) } })) }();

Issue based on this HackerNews comment by @chris-morgan: https://news.ycombinator.com/item?id=37048556

<script type="text/typogram">

This should use \<pre\> instead, e.g. <pre class="typogram">. It’s content, not scripting, and if the JavaScript isn’t run (for whatever reason—JS is less reliable than people often think, especially third-party JS, even on environments that don’t try to block it) you’d like the diagram to still be visible in some form.

My suggestion would be to use a custom data attributes (i.e. data-typogram) instead, but it might just be personal preference.

Retaining the \<pre\> would also be a great improvement for selecting text—the current arrangement of “place single-character <text> elements” is almost useless for copy and paste (losing spaces and line breaks), which is the main reason I can imagine wanting such a thing.

jwbargsten commented 3 days ago

FYI: I decided to refactor the code, including the proposed changes in the current issues. If there is still interest repo and example.