RawHTML component: Allow multiple children #35532
Conversation
andrewserong
added
[Type] Bug
|
CC @ciampo, just an FYI as this is a component used in various places. It isn't part of the Components package (so I haven't added you as a reviewer), but just thought I'd keep you in the loop 🙂 |
|
Size Change: -347 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you for the ping! Always interesting :)
I don't really have any context for this component, and so I left a few comments which hopefully aren't silly.
Also, what's the need for such a component? We should make sure that it's fed raw HTML only from trusted sources (e.g. not plugins / users / unknown API responses...)
packages/element/src/raw-html.js
Outdated
| * | ||
| * @return {JSX.Element} Dangerously-rendering component. | ||
| */ | ||
| export default function RawHTML( { children, ...props } ) { | ||
| // Concatenate into a single string if children is an array. | ||
| const rawHtml = Array.isArray( children ) ? children.join( '' ) : children; |
There was a problem hiding this comment.
It should go through React.Children utils first as explained in https://reactjs.org/docs/react-api.html#reactchildren. It's a more complex structure, so better to account for other cases if this API needs to be more flexible.
https://reactjs.org/docs/react-api.html#reactchildrentoarray
There was a problem hiding this comment.
I thought so as well. I'll close my other comment in favour of this one
There was a problem hiding this comment.
Thanks for the link, I hadn't used that particular API before, good to know! I've updated the function to cast as an array before concatenating strings.
| @@ -294,7 +294,7 @@ aside from `children` are passed. | |||
|
|
|||
| _Parameters_ | |||
|
|
|||
| - _props_ `RawHTMLProps`: Children should be a string of HTML. Other props will be passed through to div wrapper. | |||
| - _props_ `RawHTMLProps`: Children should be a string of HTML or an array of strings. Other props will be passed through to the div wrapper. | |||
There was a problem hiding this comment.
It would be great to cover this change also in the CHANGELOG file of the @wordpress/element package:
https://github.com/WordPress/gutenberg/blob/trunk/packages/element/CHANGELOG.md
There was a problem hiding this comment.
Thanks, I've added an entry. Please let me know if it needs to be more descriptive.
|
Thanks for the reviews and feedback @gziolo and @ciampo! 🙇 I've updated this PR to cast the children as an array via I've added a couple of additional unit tests, to check that it renders an empty container when passed no children, and that non-string-based children are ignored in the output, to hopefully harden the logic slightly. While I can imagine a use case for rendering out React components as plain inner html, it looks like this component was originally designed to just support being passed strings.
Good question, the component is used a fair bit within the repo, usually with trusted sources, and I see a couple of instances where the HTML is passed to a Let me know if either of you would like to see any further changes! |
|
This failing e2e test needs to be investigated:
|
This is not related and has been fixed in trunk. Just rebase. |
I have just seen it in another PR, but you were faster with reporting it back 👍🏻 |
There was a problem hiding this comment.
I've updated this PR to cast the children as an array via Children.toArray() and then check that each element is a string before concatenating. I think this is consistent with the previous expectations of how the component should behave. Is this what you had in mind?
I'll let @gziolo have the last word here, but it looks good to me
packages/element/src/raw-html.js
Outdated
| // Cast children as an array, and concatenate each element if it is a string. | ||
| Children.toArray( children ).forEach( ( child ) => { | ||
| if ( typeof child === 'string' ) { | ||
| rawHtml += child.trim(); |
There was a problem hiding this comment.
Not sure if trimming white spaces could lead to unexpected changes (see example)
There was a problem hiding this comment.
Ah, good call. The logic I was after was to only add to the rawHtml if after trimming, the string isn't empty. I'll move the trim to the if statement instead.
There was a problem hiding this comment.
I've updated this now, so the logic is how I'd personally expect it to behave (no gaps unexpectedly added in the innerHTML, but the actual strings passed to RawHTML are not trimmed). I don't feel too strongly about keeping the trim() check in, though, if folks think it's better to remove it.
There was a problem hiding this comment.
Yes, it's hard to tell what would be the best option. We can always leave it as is and update later when someone runs into an issue 😅
…oncatenating, add extra tests
…an trim passed html
andrewserong
force-pushed
the
update/raw-html-component-to-accept-multiple-children
branch
from
4291c93 to
fd0eb20
Compare
Thanks, that did the trick, the e2es are passing now. I also did a tiny update to the |
| * | ||
| * @return {JSX.Element} Dangerously-rendering component. | ||
| */ | ||
| export default function RawHTML( { children, ...props } ) { | ||
| // The DIV wrapper will be stripped by serializer, unless there are |
There was a problem hiding this comment.
Should we leave this comment? It sounds like something important.
There was a problem hiding this comment.
It was removed because it didn't reflect the behaviour of the component — see #35532 (comment)
There was a problem hiding this comment.
It's still true:
gutenberg/packages/element/src/serialize.js
Lines 387 to 396 in 80112e4
There was a problem hiding this comment.
I see. We both got confused by the comment and thought that by "serialiser" the comment referred to React's internal serialiser when calling createElement a few lines below.
We should put this comment back (and maybe make it more explicit about what is meant by "serializer")
There was a problem hiding this comment.
Yes, it should be further clarified.
There was a problem hiding this comment.
Excellent, thanks for clarifying. I've restored the comment and reworded it so that it's clear which serializer we're talking about. Glad we got to the bottom of that!
There was a problem hiding this comment.
@andrewserong, I think it looks good and it should remain backward compatible.
…ify which serializer we mean
andrewserong
deleted the
update/raw-html-component-to-accept-multiple-children
branch
|
Thank you @andrewserong and @ciampo for wrangling this enhancement 👍🏻 |
|
Pleasure, thanks for the review and feedback! 🙇 |
Description
This PR updates the RawHTML component to support passing multiple children instead of treating
childrenas a string.This means that if you wish to use the RawHTML component and include raw HTML from a few different variables, you don't need to manually concatenate those string before passing them to the component.
I have also updated the tests for the RawHTML component so that it uses React Testing Library and tests the output of the component, instead of the internal state of the React element.
How has this been tested?
Running tests:
For a concrete example of the issue that I ran into that led to this PR, I was playing around with adding an extra string to the
RawHTMLcomponent used in the post content block's ReadOnlyContent component (rendered when a Post Content block appears as a child of the Query loop block). Around this line.Example code:
The above code, before this PR, results in stray
,characters being rendered, as thechildrenarray is joined into a string using,characters as a separator. With this PR applied, we concatenate the strings before passing todangerouslySetInnerHTMLso there are no stray comma characters.If you wish to reproduce this, use the above code in line 36 of
/packages/block-library/src/post-content/edit.jsand using a block-based theme like TT1-Blocks, publish a post. Then, open up the site editor and view the post content block within a Query loop block. You should be able to then see the differences in the before/after screenshots below.Screenshots
Note the stray comma characters in the Before screenshot, and that they no longer appear in the After screenshot.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).