fix(react): heading with links not rendered #41

jpedroschmitz · 2021-10-14T20:10:16Z

Closes MAR-2635

changeset-bot · 2021-10-14T20:10:18Z

🦋 Changeset detected

Latest commit: ef595a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@graphcms/rich-text-react-renderer	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

linear · 2021-10-14T20:10:19Z

MAR-2635 Heading links not being rendered

User report:

Hey there, our client ran into an issue yesterday regarding some Rich Text formatting/rendering.
They were trying to have an H2 as a link, and it was simply not displaying at all (we use your library to render the content). After an investigation, we found that it was related to a rule that the library has regarding SEO concerns on elements like headings, where, if the text is empty, you just won't render it. That is perfectly understandable, but there's a slight issue with it: the way this "emptiness" is checked is by looking at children[0].text and checking if it is an empty string (line here).
The reason why this is an issue is, whenever a link is created (at least on its own line/node), we'd expected the children of the paragraph to only contain the link itself, but instead, it contains a first child with an empty text, the link, and then another empty text child. This is causing the H2 to not render because the library only checks for the 1st child emptiness.
Questions/Suggestions : Is there something we're doing wrong ? If not, could you maybe update that rule to check for all children's emptiness instead ?

github-actions · 2021-10-14T20:12:21Z

size-limit report 📦

Path	Size
packages/react-renderer/dist/rich-text-react-renderer.cjs.production.min.js	4.96 KB (+1.16% 🔺)
packages/react-renderer/dist/rich-text-react-renderer.esm.js	4.94 KB (+1.53% 🔺)
packages/types/dist/rich-text-types.cjs.production.min.js	51 B (0%)
packages/types/dist/rich-text-types.esm.js	64 B (0%)

feychenie

Looking good for the reported bug 👍🏻

Maybe in a second time it could be even more resilient by handling unknown levels of nesting instead of just the first one 🤔

feychenie · 2021-10-14T21:17:06Z

packages/html-to-slate-ast/src/index.ts

@@ -6,7 +6,7 @@ import {
 } from 'slate';
 import { jsx } from 'slate-hyperscript';
 import { sanitizeUrl } from '@braintree/sanitize-url';
-import type { Element, Mark } from '@graphcms/rich-text-types';


🤔 did you change your autoformater config?
import type is better and becoming the "best practice"

Or is one of these not a TS type but an actual Class?

I had to remove it 😓

There was a problem with ESLint not recognizing it as a valid import. The only fix was removing it because I couldn't update TSDX ESLint version. I tried fixing it with Yarn resolutions, but it didn't work.

We probably need to move away from TSDX. The project is abandoned. See jaredpalmer/tsdx#1065.

feychenie · 2021-10-14T21:19:36Z

packages/react-renderer/src/RichText.tsx

   *
   * Elements that can be removed with empty text are defined in `defaultRemoveEmptyElements`
   */
  if (
    defaultRemoveEmptyElements?.[
      elementKeys[type] as keyof RemoveEmptyElementType
    ] &&
-    children[0].text === ''
+    elementIsEmpty({ children })


hard to distinguish with all the formatting changes, is that -with the separate function definition- the actual/only fix of this PR? (makes sense to me, just making sure i did not miss anything else among the noise)

Yeah, it's the only fix. I created this small utility function to check if an element is empty, so we can reuse it if needed.

feychenie · 2021-10-14T21:23:30Z

packages/react-renderer/src/util/elementIsEmpty.ts

+export function elementIsEmpty({
+  children,
+}: {
+  children: (ElementNode | Text)[];
+}): boolean {
+  // Checks if the children array has more than one element.
+  // It may have a link inside, that's why we need to check this condition.
+  if (children.length > 1) {
+    const hasText = children.filter(function f(child): boolean | number {
+      if (isText(child) && child.text !== '') {
+        return true;
+      }
+
+      if (isElement(child)) {
+        return (child.children = child.children.filter(f)).length;
+      }
+
+      return false;
+    });
+
+    return hasText.length > 0 ? false : true;
+  } else if (children[0].text === '') return true;
+
+  return true;
+}


should we maybe use recursion here? I mean what if there is deeper nesting?

I can't think of a use case where we have deeper nesting with the children's object, but this function will check if there's more than one item in the array of children. If it has, it will filter it using recursion. Note that the callback of the filter method is a function that we reuse if the node is an element. If it's a text and it's not empty, we return true.

When there's text inside it, the hasText variable will contain the elements with text, meaning the element is not empty.

I think that's it.

jpedroschmitz added 4 commits October 14, 2021 14:12

test: make sure heading with links are rendered

a86764d

chore: fix eslint issue

e36c181

style: run prettier

336b282

fix(react): heading with links not rendered

ccd701d

chore: add changeset

ef595a4

jpedroschmitz requested a review from notrab October 14, 2021 20:11

feychenie approved these changes Oct 14, 2021

View reviewed changes

jpedroschmitz merged commit c7ea848 into main Oct 15, 2021

jpedroschmitz deleted the fix/heading-with-link branch October 15, 2021 16:51

github-actions bot mentioned this pull request Oct 15, 2021

chore(release): publish #43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(react): heading with links not rendered #41

fix(react): heading with links not rendered #41

jpedroschmitz commented Oct 14, 2021

changeset-bot bot commented Oct 14, 2021 •

edited

Loading

linear bot commented Oct 14, 2021

github-actions bot commented Oct 14, 2021 •

edited

Loading

feychenie left a comment

feychenie Oct 14, 2021

feychenie Oct 14, 2021 •

edited

Loading

jpedroschmitz Oct 15, 2021

feychenie Oct 14, 2021

jpedroschmitz Oct 15, 2021

feychenie Oct 14, 2021 •

edited

Loading

jpedroschmitz Oct 15, 2021

fix(react): heading with links not rendered #41

fix(react): heading with links not rendered #41

Conversation

jpedroschmitz commented Oct 14, 2021

changeset-bot bot commented Oct 14, 2021 • edited Loading

🦋 Changeset detected

linear bot commented Oct 14, 2021

github-actions bot commented Oct 14, 2021 • edited Loading

size-limit report 📦

feychenie left a comment

Choose a reason for hiding this comment

feychenie Oct 14, 2021

Choose a reason for hiding this comment

feychenie Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

jpedroschmitz Oct 15, 2021

Choose a reason for hiding this comment

feychenie Oct 14, 2021

Choose a reason for hiding this comment

jpedroschmitz Oct 15, 2021

Choose a reason for hiding this comment

feychenie Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

jpedroschmitz Oct 15, 2021

Choose a reason for hiding this comment

changeset-bot bot commented Oct 14, 2021 •

edited

Loading

github-actions bot commented Oct 14, 2021 •

edited

Loading

feychenie Oct 14, 2021 •

edited

Loading

feychenie Oct 14, 2021 •

edited

Loading