fix: updates to html block #906

jennspencer · 2024-06-16T23:24:27Z

	Fix RM-10043

🧰 Changes

Reworks HTML blocks a bit so they're nicer and easier for the user to interact with (makes sure the HTML is always nicely formatted for slate and the markdown/mdx doc)

🧬 QA & Testing

components/HTMLBlock/index.tsx

rafegoldberg

Looking good! Had some questions/suggestions, but nothing major.

rafegoldberg · 2024-06-17T14:50:35Z

processor/compile/html-block.ts

-  return `<HTMLBlock${attributes && ' ' + attributes}>{\`${ html }\`}</HTMLBlock>`;
+  const { runScripts, html } = getHProps<HTMLBlock['data']['hProperties']>(node);
+
+  return `<HTMLBlock${runScripts != null ? ' runScripts="' + runScripts + '"' : ''}>{\`\n${ reformatHTML(html) }\n\`}</HTMLBlock>`;


If we're converting this to use a template string, we should probably also make it easier to read while we're at it:

Suggested change

return `<HTMLBlock${runScripts != null ? ' runScripts="' + runScripts + '"' : ''}>{\`\n${ reformatHTML(html) }\n\`}</HTMLBlock>`;

return `<HTMLBlock ${runScripts != null ? `runScripts="${runScripts}"` : ''}>{\`

${reformatHTML(html)}

\`}</HTMLBlock>`;

sure! although i'm aesthetically offended by the extra space that's left if there's no runScripts prop 😂

rafegoldberg · 2024-06-17T14:53:18Z

processor/compile/html-block.ts

-  return `<HTMLBlock${attributes && ' ' + attributes}>{\`${ html }\`}</HTMLBlock>`;
+  const { runScripts, html } = getHProps<HTMLBlock['data']['hProperties']>(node);
+
+  return `<HTMLBlock${runScripts != null ? ' runScripts="' + runScripts + '"' : ''}>{\`\n${ reformatHTML(html) }\n\`}</HTMLBlock>`;


Ahhh, so it looks like you're setting the runScripts prop to a string on compile here. I'll betchya anything that's the reason it's sometimes coming in as a string, per my above question? Can we update this to be a JSX-y value instead?

- `runScripts="${runScripts}"` + `runScripts={${runScripts}}`

If I'm understanding correctly, I believe ^that should resolve to true rather than "true"!

for some reason MDX was treating {true} as an object. it's on my list to work out, but i had just put this in as a stop-gap so the functionality could be tested!

processor/utils.ts

types.d.ts

kellyjosephprice

Some questions and I really think we shouldn't munge the html value.

processor/transform/readme-components.ts

processor/utils.ts

kellyjosephprice · 2024-06-18T17:42:40Z

processor/transform/readme-components.ts

+      const { position } = node;
+      const children = getChildren<HTMLBlock['children']>(node);
+      const { runScripts } = getAttrs<Pick<HTMLBlock['data']['hProperties'], 'runScripts'>>(node);
+      const html = formatHTML(children.map(({ value }) => value).join(''));


I think trimming the content is fine, but I worry that changing the indent could be problematic. I'm inclined to say we shouldn't munge the string beyond trimming leading and trailing whitespce?

yeah i'm not a big fan of the munging, which is why i tried to keep it to a minimum. if you just .trim(), the raw HTML displays in a messy way in the widget code editor/the recompiled markdown :(

What kind of messy way, cause it seems wrong to me: https://github.com/readmeio/markdown/pull/910/files#diff-3abd1a08f8ea2dfcc23db22057e28a7278deda6fb0f0e81583b1babe581edf76R17-R29

it reindents the HTML so it's not at the same indentation as the main HTMLBlock component. i admit it was a subjective style choice, but it helps make the md doc easier to read?

(while also keeping the original structure as much as possible)

(and i totally admit that there are a few scenarios here i need to account for)

| [![PR App][icn]][demo] | Fix RM-XYZ | | :--------------------: | :--------: | ## 🧰 Changes Describe your changes in detail. ## 🧬 QA & Testing - [Broken on production][prod]. - [Working in this PR app][demo]. [demo]: https://markdown-pr-PR_NUMBER.herokuapp.com [prod]: https://SUBDOMAIN.readme.io [icn]: https://user-images.githubusercontent.com/886627/160426047-1bee9488-305a-4145-bb2b-09d8b757d38a.svg

kellyjosephprice

Code looks good! But I think the failing test with whitespace is a blocker. And I'm not sure I understand the motivation for this???

I understand trimming so we can get something like:

<HTMLBlock>{`
...
`}</HTMLBlock

That's nice and good. But I'm still not keen on messing with the internal whitespace. I think leaving it unindented would be fine:

<HTMLBlock>{`
<h2>
  <img>
  Hello!
</h2>
`}</HTMLBlock>

Is there something I'm missing with regards to the editor? The thing that makes this important to me, is that if we start applying a particular formatting to this block, and it turns out to be a problem later, we'd probably have to run a big migration to fix it.

jennspencer · 2024-06-20T19:51:55Z

@kellyjosephprice i stg greg made a comment somewhere that he preferred the scenario i was trying to munge, but i can't find anywhere it so it's possible i'm losing my mind.

if the user does decide to indent the html in the HTMLBlock, it'll show up in the widget's code editor with the indents though. is that enough of an issue to do something about or just leave it? i'm wildly ambivalent, mostly because users probably tend to stick with their preferred way of editing (editor vs raw markdown)

oh and i'm totally fixing the test, just had to figure out what was the "correct" outcome of it first 😂

kellyjosephprice

Sorry and thank you!

addressed changes

## Version 6.75.0-beta.60 ### 🛠 Fixes & Updates * updates to html block ([#906](#906)) ([ddc97db](ddc97db))

rafegoldberg · 2024-06-20T21:41:33Z

This PR was released!

🚀 Changes included in v6.75.0-beta.60

jennspencer added 3 commits June 14, 2024 19:31

fix: process HTML block

a04b5d2

html block compiler tweaks

5a543fc

make nicer

b64caca

jennspencer requested review from kellyjosephprice and rafegoldberg June 16, 2024 23:24

stricter testing

023c2e7

rafegoldberg reviewed Jun 17, 2024

View reviewed changes

components/HTMLBlock/index.tsx Show resolved Hide resolved

rafegoldberg previously requested changes Jun 17, 2024

View reviewed changes

i herd u like comments

f5cb19f

jennspencer requested a review from rafegoldberg June 17, 2024 20:46

jennspencer added 3 commits June 17, 2024 13:59

fix

7e0233f

revert tag name

3e54110

undo extra tab

35c38e3

kellyjosephprice requested changes Jun 18, 2024

View reviewed changes

jennspencer requested a review from kellyjosephprice June 20, 2024 17:44

kellyjosephprice reviewed Jun 20, 2024

View reviewed changes

jennspencer requested a review from kellyjosephprice June 20, 2024 19:59

unmunge html

e3b63d1

kellyjosephprice approved these changes Jun 20, 2024

View reviewed changes

jennspencer merged commit ddc97db into beta Jun 20, 2024
9 of 11 checks passed

jennspencer deleted the jenn/rm-10043-custom-html-blocks-not-rendering-in-editor branch June 20, 2024 21:38

rafegoldberg pushed a commit that referenced this pull request Jun 20, 2024

build(release): 🚀 v6.75.0-beta.60 🦉

bdc54a2

## Version 6.75.0-beta.60 ### 🛠 Fixes & Updates * updates to html block ([#906](#906)) ([ddc97db](ddc97db))

rafegoldberg added the released on @beta label Jun 20, 2024

kellyjosephprice mentioned this pull request Jul 9, 2024

chore: profiling #936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: updates to html block #906

fix: updates to html block #906

jennspencer commented Jun 16, 2024

rafegoldberg left a comment

rafegoldberg Jun 17, 2024 •

edited

Loading

jennspencer Jun 17, 2024

rafegoldberg Jun 17, 2024 •

edited

Loading

jennspencer Jun 17, 2024

kellyjosephprice left a comment

kellyjosephprice Jun 18, 2024

jennspencer Jun 18, 2024

kellyjosephprice Jun 18, 2024

jennspencer Jun 18, 2024

jennspencer Jun 18, 2024

jennspencer Jun 18, 2024

kellyjosephprice left a comment

jennspencer commented Jun 20, 2024 •

edited

Loading

kellyjosephprice left a comment

rafegoldberg commented Jun 20, 2024

fix: updates to html block #906

fix: updates to html block #906

Conversation

jennspencer commented Jun 16, 2024

🧰 Changes

🧬 QA & Testing

rafegoldberg left a comment

Choose a reason for hiding this comment

rafegoldberg Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafegoldberg Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyjosephprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyjosephprice left a comment

Choose a reason for hiding this comment

jennspencer commented Jun 20, 2024 • edited Loading

kellyjosephprice left a comment

Choose a reason for hiding this comment

rafegoldberg commented Jun 20, 2024

This PR was released!

rafegoldberg Jun 17, 2024 •

edited

Loading

rafegoldberg Jun 17, 2024 •

edited

Loading

jennspencer commented Jun 20, 2024 •

edited

Loading