Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Oct 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/yellow-shirts-stare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@graphcms/rich-text-react-renderer': patch
---

Fix heading with links not being rendered
8 changes: 2 additions & 6 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
module.exports = {
extends: [
'react-app',
'prettier/@typescript-eslint',
'plugin:prettier/recommended',
],
plugins: ['testing-library', 'jest-dom'],
extends: ['react-app', 'prettier/@typescript-eslint', 'prettier'],
plugins: ['testing-library', 'jest-dom', 'prettier'],
settings: {
react: {
version: '999.999.999',
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
"husky": "^6.0.0",
"lerna": "^3.15.0",
"lint-staged": "^11.0.0",
"prettier": "^2.3.0",
"react": "^17.0.2",
"react-dom": "^17.0.2",
"size-limit": "^4.10.2",
Expand Down
32 changes: 16 additions & 16 deletions packages/html-to-slate-ast/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@feychenie feychenie Oct 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import { Element, Mark } from '@graphcms/rich-text-types';

const ELEMENT_TAGS: Record<
HTMLElement['nodeName'],
Expand All @@ -16,7 +16,7 @@ const ELEMENT_TAGS: Record<
OL: () => ({ type: 'numbered-list' }),
UL: () => ({ type: 'bulleted-list' }),
P: () => ({ type: 'paragraph' }),
A: (el) => {
A: el => {
const href = el.getAttribute('href');
if (href === null) return {};
return {
Expand Down Expand Up @@ -44,7 +44,7 @@ const ELEMENT_TAGS: Record<
TR: () => ({ type: 'table_row' }),
TD: () => ({ type: 'table_cell' }),
TH: () => ({ type: 'table_cell' }),
IMG: (el) => {
IMG: el => {
const href = el.getAttribute('src');
const title = Boolean(el.getAttribute('alt'))
? el.getAttribute('alt')
Expand Down Expand Up @@ -108,7 +108,7 @@ function deserialize<
parent = el.childNodes[0];
}
let children = Array.from(parent.childNodes)
.map((c) => deserialize(c, global))
.map(c => deserialize(c, global))
.flat();

if (children.length === 0) {
Expand All @@ -124,7 +124,7 @@ function deserialize<
if (
isElementNode(el) &&
Array.from(el.attributes).find(
(attr) => attr.name == 'role' && attr.value === 'heading'
attr => attr.name === 'role' && attr.value === 'heading'
)
) {
const level = el.attributes.getNamedItem('aria-level')?.value;
Expand Down Expand Up @@ -190,7 +190,7 @@ function deserialize<
children: [{ text: '' }],
},
]
: childNodes.map((child) => ({
: childNodes.map(child => ({
type: 'paragraph',
children: [{ text: child.textContent ? child.textContent : '' }],
}));
Expand All @@ -204,7 +204,7 @@ function deserialize<
if (nodeName === 'DIV') {
const childNodes = Array.from(el.childNodes);
const isParagraph = childNodes.every(
(child) =>
child =>
(isElementNode(child) && isInlineElement(child)) || isTextNode(child)
);
if (isParagraph) {
Expand Down Expand Up @@ -240,36 +240,36 @@ function deserialize<
})();
if (tagNames) {
const attrs = tagNames.reduce((acc, current) => {
return ({...acc, ...TEXT_TAGS[current]() });
return { ...acc, ...TEXT_TAGS[current]() };
}, {});
return children.map((child) => {
return children.map(child => {
if (typeof child === 'string') {
return jsx('text', attrs, child);
}

if (isChildNode(child, global)) return child;

if (SlateElement.isElement(child) && !SlateText.isText(child)) {
child.children = child.children.map((c) => ({ ...c, ...attrs }));
child.children = child.children.map(c => ({ ...c, ...attrs }));
return child;
}

return child;
});
}
}

if (TEXT_TAGS[nodeName]) {
const attrs = TEXT_TAGS[nodeName](el as HTMLElement);
return children.map((child) => {
return children.map(child => {
if (typeof child === 'string') {
return jsx('text', attrs, child);
}

if (isChildNode(child, global)) return child;

if (SlateElement.isElement(child) && !SlateText.isText(child)) {
child.children = child.children.map((c) => ({ ...c, ...attrs }));
child.children = child.children.map(c => ({ ...c, ...attrs }));
return child;
}

Expand Down Expand Up @@ -441,7 +441,7 @@ export async function htmlToSlateAST(html: string) {
const domDocument = await parseDomDocument(normalizedHTML);
const global = await (async () => {
if (typeof window !== 'undefined') return window;
return await import('jsdom').then((jsdom) => new jsdom.JSDOM().window);
return await import('jsdom').then(jsdom => new jsdom.JSDOM().window);
})();
return deserialize(domDocument.body, global);
}
Expand Down
31 changes: 18 additions & 13 deletions packages/html-to-slate-ast/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ test('Transforms top level span into paragraph', () => {
in fact, is the very CSS and HTML rendered as I type this blog. There are calls to HTML element classes that style
certain properties. For example, the font-family properties in the ".postArticle-content .graf — p" class has a serif
font value, hence the text rendered in this article is of the serif family. All this to say, if you as a pro</span>`).then(
(ast) =>
ast =>
expect(ast).toEqual([
{
type: 'paragraph',
Expand All @@ -30,7 +30,7 @@ test('Transforms inner span into paragraph', () => {
in fact, is the very CSS and HTML rendered as I type this blog. There are calls to HTML element classes that style
certain properties. For example, the font-family properties in</span><span> the ".postArticle-content .graf — p" class has a serif
font value, hence the text rendered in this article is of the serif family. All this to say, if you as a pro</span></p>`).then(
(ast) =>
ast =>
expect(ast).toEqual([
{
type: 'paragraph',
Expand All @@ -49,7 +49,7 @@ test('Transforms inner span into paragraph', () => {

test('Transforms inner spans wrapped in a div into paragraph', () => {
const input = fs.readFileSync(__dirname + '/html_input.html').toString();
return htmlToSlateAST(input).then((ast) =>
return htmlToSlateAST(input).then(ast =>
expect(ast).toStrictEqual([
{
type: 'paragraph',
Expand Down Expand Up @@ -91,7 +91,7 @@ test('Transforms Google Docs input', () => {
const input = fs
.readFileSync(__dirname + '/google-docs_input.html')
.toString();
return htmlToSlateAST(input).then((ast) =>
return htmlToSlateAST(input).then(ast =>
expect(ast).toEqual([
{
type: 'heading-one',
Expand Down Expand Up @@ -362,12 +362,14 @@ test('Transforms Google Docs input', () => {
children: [
{
type: 'link',
href: 'https://lh6.googleusercontent.com/TkJFBZvkyXTa602F0gkp2phU0O1eHu96RdKFcQ8l_EOS_CBfcI9jYRixN6sNRFnFiZ-ssbLbnLDReb3FrEZ1MnLr70c5gIvPmhJtV7appyVEDSeHLIRdNwdNzbIqs3l2GOgGLGC5=s0',
href:
'https://lh6.googleusercontent.com/TkJFBZvkyXTa602F0gkp2phU0O1eHu96RdKFcQ8l_EOS_CBfcI9jYRixN6sNRFnFiZ-ssbLbnLDReb3FrEZ1MnLr70c5gIvPmhJtV7appyVEDSeHLIRdNwdNzbIqs3l2GOgGLGC5=s0',
title: 'Screenshot 2021-06-10 at 15.56.22.png',
openInNewTab: true,
children: [
{
text: 'https://lh6.googleusercontent.com/TkJFBZvkyXTa602F0gkp2phU0O1eHu96RdKFcQ8l_EOS_CBfcI9jYRixN6sNRFnFiZ-ssbLbnLDReb3FrEZ1MnLr70c5gIvPmhJtV7appyVEDSeHLIRdNwdNzbIqs3l2GOgGLGC5=s0',
text:
'https://lh6.googleusercontent.com/TkJFBZvkyXTa602F0gkp2phU0O1eHu96RdKFcQ8l_EOS_CBfcI9jYRixN6sNRFnFiZ-ssbLbnLDReb3FrEZ1MnLr70c5gIvPmhJtV7appyVEDSeHLIRdNwdNzbIqs3l2GOgGLGC5=s0',
},
],
},
Expand All @@ -388,7 +390,7 @@ test('Transforms Google Docs input', () => {
test('Converts word documents', () => {
return htmlToSlateAST(
fs.readFileSync(__dirname + '/word-document.html').toString()
).then((ast) =>
).then(ast =>
expect(ast).toStrictEqual([
{
type: 'heading-one',
Expand Down Expand Up @@ -580,16 +582,18 @@ test('Converts word documents', () => {
test('Converts an image pasted from Google Docs into a link node', () => {
return htmlToSlateAST(
fs.readFileSync(__dirname + '/image.html').toString()
).then((ast) =>
).then(ast =>
expect(ast).toStrictEqual([
{
type: 'link',
href: 'https://lh5.googleusercontent.com/EqByyE2l_VVSU6KoXFlkpPjJIBsbMTb4Dkr0cuvy2K5ctn8BoJsDHBXO0rU2wyck72_ZF1rqJ5kJ0iMEjU4Jwf7mKhRaLWoHJAzX5WvpfMytIR9sw3EwBcdQdRlIwSrsQ3odhUYq',
href:
'https://lh5.googleusercontent.com/EqByyE2l_VVSU6KoXFlkpPjJIBsbMTb4Dkr0cuvy2K5ctn8BoJsDHBXO0rU2wyck72_ZF1rqJ5kJ0iMEjU4Jwf7mKhRaLWoHJAzX5WvpfMytIR9sw3EwBcdQdRlIwSrsQ3odhUYq',
title: "this is this image's title",
openInNewTab: true,
children: [
{
text: 'https://lh5.googleusercontent.com/EqByyE2l_VVSU6KoXFlkpPjJIBsbMTb4Dkr0cuvy2K5ctn8BoJsDHBXO0rU2wyck72_ZF1rqJ5kJ0iMEjU4Jwf7mKhRaLWoHJAzX5WvpfMytIR9sw3EwBcdQdRlIwSrsQ3odhUYq',
text:
'https://lh5.googleusercontent.com/EqByyE2l_VVSU6KoXFlkpPjJIBsbMTb4Dkr0cuvy2K5ctn8BoJsDHBXO0rU2wyck72_ZF1rqJ5kJ0iMEjU4Jwf7mKhRaLWoHJAzX5WvpfMytIR9sw3EwBcdQdRlIwSrsQ3odhUYq',
},
],
},
Expand All @@ -600,7 +604,7 @@ test('Converts an image pasted from Google Docs into a link node', () => {
test('Reshape an incorrectly structured table', () => {
return htmlToSlateAST(
'<table><colgroup><col /><col /></colgroup><tbody><tr><td></td></tr><tr></tr></tbody></table>'
).then((ast) =>
).then(ast =>
expect(ast).toStrictEqual([
{
type: 'table',
Expand Down Expand Up @@ -654,13 +658,14 @@ test('Reshape an incorrectly structured table', () => {

test('Transforms pre tags into code-block nodes', () => {
const input = fs.readFileSync(__dirname + '/pre.html').toString();
return htmlToSlateAST(input).then((ast) =>
return htmlToSlateAST(input).then(ast =>
expect(ast).toStrictEqual([
{
type: 'code-block',
children: [
{
text: " L TE\n A A\n C V\n R A\n DOU\n LOU\n REUSE\n QUE TU\n PORTES\n ET QUI T'\n ORNE O CI\n VILISÉ\n OTE- TU VEUX\n LA BIEN\n SI RESPI\n RER - Apollinaire",
text:
" L TE\n A A\n C V\n R A\n DOU\n LOU\n REUSE\n QUE TU\n PORTES\n ET QUI T'\n ORNE O CI\n VILISÉ\n OTE- TU VEUX\n LA BIEN\n SI RESPI\n RER - Apollinaire",
},
],
},
Expand Down
11 changes: 5 additions & 6 deletions packages/react-renderer/src/RichText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
} from './defaultElements';
import { RenderText } from './RenderText';
import { getElements } from './util/getElements';
import { elementIsEmpty } from './util/elementIsEmpty';

function RenderNode({
node,
Expand Down Expand Up @@ -65,15 +66,15 @@ function RenderElement({
const { nodeId, nodeType } = rest;

/**
* Checks if element has empty text, so it can be removed.
* Checks if the element is empty, so that it can be removed.
*
* Elements that can be removed with empty text are defined in `defaultRemoveEmptyElements`
*/
if (
defaultRemoveEmptyElements?.[
elementKeys[type] as keyof RemoveEmptyElementType
] &&
children[0].text === ''
elementIsEmpty({ children })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
return <Fragment />;
}
Expand All @@ -85,7 +86,7 @@ function RenderElement({
* Since there won't be duplicated ID's, it's safe to use the first element.
*/
const referenceValues = isEmbed
? references?.filter((ref) => ref.id === nodeId)[0]
? references?.filter(ref => ref.id === nodeId)[0]
: null;

/**
Expand Down Expand Up @@ -251,9 +252,7 @@ export function RichText({
If it isn't defined and there's embed elements, it will show a warning
*/
if (__DEV__) {
const embedElements = elements.filter(
(element) => element.type === 'embed'
);
const embedElements = elements.filter(element => element.type === 'embed');

if (embedElements.length > 0 && !references) {
console.warn(
Expand Down
6 changes: 3 additions & 3 deletions packages/react-renderer/src/defaultElements.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ export const defaultElements: Required<NodeRendererType> = {
),
list_item_child: ({ children }) => <>{children}</>,
Asset: {
audio: (props) => <Audio {...props} url={props.url} />,
image: (props) => <Image {...props} src={props.url} />,
video: (props) => <Video {...props} src={props.url} />,
audio: props => <Audio {...props} url={props.url} />,
image: props => <Image {...props} src={props.url} />,
video: props => <Video {...props} src={props.url} />,
font: FallbackForCustomAsset,
application: FallbackForCustomAsset,
model: FallbackForCustomAsset,
Expand Down
32 changes: 32 additions & 0 deletions packages/react-renderer/src/util/elementIsEmpty.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import {
ElementNode,
isElement,
isText,
Text,
} from '@graphcms/rich-text-types';

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;
}
Comment on lines +8 to +32
Copy link
Contributor

@feychenie feychenie Oct 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

19 changes: 18 additions & 1 deletion packages/react-renderer/test/RichText.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,28 @@ describe('@graphcms/rich-text-react-renderer', () => {
`);
});

it('renders content correctly if received a object with empty children', () => {
it('should not render elements if received a object with empty children', () => {
const { container } = render(<RichText content={emptyContent} />);

expect(container).toMatchInlineSnapshot(`
<div>
<h2>

<a
href="https://graphcms.com"
>
Testing Link
</a>
</h2>
<h2>

<a
href="https://graphcms.com"
>
Link
</a>
2
</h2>
<table>
<tbody>
<tr>
Expand Down
Loading