fix(react): heading with links not rendered #41
Conversation
🦋 Changeset detectedLatest commit: ef595a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
MAR-2635 Heading links not being rendered
User report:
|
size-limit report 📦
|
| @@ -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'; | |||
There was a problem hiding this comment.
🤔 did you change your autoformater config?
import type is better and becoming the "best practice"
There was a problem hiding this comment.
Or is one of these not a TS type but an actual Class?
There was a problem hiding this comment.
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.
| * | ||
| * Elements that can be removed with empty text are defined in `defaultRemoveEmptyElements` | ||
| */ | ||
| if ( | ||
| defaultRemoveEmptyElements?.[ | ||
| elementKeys[type] as keyof RemoveEmptyElementType | ||
| ] && | ||
| children[0].text === '' | ||
| elementIsEmpty({ children }) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
should we maybe use recursion here? I mean what if there is deeper nesting?
There was a problem hiding this comment.
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.
Closes MAR-2635