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 suggested 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

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

fix: updates to html block #906

fix: updates to html block #906

Uh oh!

Conversation

jennspencer commented Jun 16, 2024

🧰 Changes

🧬 QA & Testing

Uh oh!

Uh oh!

rafegoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

rafegoldberg Jun 17, 2024 • 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!

rafegoldberg Jun 17, 2024 • 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!

Uh oh!

Uh oh!

kellyjosephprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

kellyjosephprice left a comment

Choose a reason for hiding this comment

Uh oh!

jennspencer commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kellyjosephprice left a comment

Choose a reason for hiding this comment

Uh oh!

rafegoldberg commented Jun 20, 2024

This PR was released!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rafegoldberg Jun 17, 2024 •

edited

Loading

rafegoldberg Jun 17, 2024 •

edited

Loading

jennspencer commented Jun 20, 2024 •

edited

Loading