Make sure comments keep their location when inside paragraph #31374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vdwijngaert
added
[Type] Bug
|
Size Change: +142 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
@gwwar Kerry can you take a look? |
vdwijngaert
force-pushed
the
fix/more-comment-converter-inside-paragraph
branch
from
0a95688 to
698e018
Compare
gwwar
reviewed
May 10, 2021
There was a problem hiding this comment.
This one manually tests well for me @vdwijngaert ! Let's get another +1 from folks a little more familiar with this package. From recent history, perhaps @ellatrix or @mcsf could 👀 on code changes here.
| * | ||
| * @param {Node} node The node to be processed. | ||
| * @param {Document} doc The document of the node. | ||
| * @return {void} |
There was a problem hiding this comment.
Optional: Note that if we return {void} it's fine to leave the return off JSDoc.
| @@ -49,6 +49,44 @@ describe( 'specialCommentConverter', () => { | |||
| <p>Third paragraph</p>` | |||
| ); | |||
| } ); | |||
| describe( 'when more comment is inside paragraph', () => { | |||
There was a problem hiding this comment.
👏 Thanks for adding useful test cases!
| childNodes.slice( 0, nodeIndex ).reduce( paragraphBuilder, null ), | ||
| moreBlock, | ||
| childNodes.slice( nodeIndex + 1 ).reduce( paragraphBuilder, null ), | ||
| ].forEach( |
There was a problem hiding this comment.
Do folks know if we've polyfilled https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/replaceWith ? It's 💯 optional, but might be nice here.
eg
paragraphWithMore.replaceWith( firstParagraph, moreBlock, secondParagraph );| const nodeIndex = childNodes.indexOf( node ); | ||
| const wrapperNode = node.parentNode.parentNode || doc.body; | ||
|
|
||
| const paragraphBuilder = ( acc, child ) => { |
There was a problem hiding this comment.
I can read this okay, but do other reviewers prefer something more declarative? The reducer here did stick out a little bit.
There was a problem hiding this comment.
Yeah, it's not a big deal, but the reducer feels unnecessarily abstract. I don't mind the use of reduce itself, but it might be clearer to use its initialValue argument and inline the reducer, e.g.
childNodes.slice( 0, nodeIndex ).reduce(
( acc, child ) => { acc.appendChild( child ); return acc; },
doc.createElement( 'p' )
);prefer something more declarative?
The other option, in my head, kind of goes in the opposite direction: making this clearer by following local conventions. In raw-handling, a lot of DOM manipulations are done very imperatively, mostly because performance is paramount but also because it's also syntactically convenient, especially iterating through or unwrapping child nodes. So paradoxically, in this module, it might be more readable to write something like:
for ( let i = 0; i < nodeIndex; i++ ) insert( childNodes[i], node.parentNode );
insert( moreBlock, node.parentNode );
for ( let i = nodeIndex; i < l; i++ ) insert( childNodes[i], node.parentNode );I have no idea if modern JIT compilers can optimise [ e0, e1, ... ].forEach( fn ) into something that doesn't require allocating memory for that temporary array (e.g. optimise it as a loop). So I can't comment on the performance impact of what we're doing with Array.from+Array#reduce in this PR, and whether it matters at all. :)
mcsf
approved these changes
May 12, 2021
packages/blocks/src/api/raw-handling/test/special-comment-converter.js
Outdated
Show resolved
Hide resolved
| const nodeIndex = childNodes.indexOf( node ); | ||
| const wrapperNode = node.parentNode.parentNode || doc.body; | ||
|
|
||
| const paragraphBuilder = ( acc, child ) => { |
There was a problem hiding this comment.
Yeah, it's not a big deal, but the reducer feels unnecessarily abstract. I don't mind the use of reduce itself, but it might be clearer to use its initialValue argument and inline the reducer, e.g.
childNodes.slice( 0, nodeIndex ).reduce(
( acc, child ) => { acc.appendChild( child ); return acc; },
doc.createElement( 'p' )
);prefer something more declarative?
The other option, in my head, kind of goes in the opposite direction: making this clearer by following local conventions. In raw-handling, a lot of DOM manipulations are done very imperatively, mostly because performance is paramount but also because it's also syntactically convenient, especially iterating through or unwrapping child nodes. So paradoxically, in this module, it might be more readable to write something like:
for ( let i = 0; i < nodeIndex; i++ ) insert( childNodes[i], node.parentNode );
insert( moreBlock, node.parentNode );
for ( let i = nodeIndex; i < l; i++ ) insert( childNodes[i], node.parentNode );I have no idea if modern JIT compilers can optimise [ e0, e1, ... ].forEach( fn ) into something that doesn't require allocating memory for that temporary array (e.g. optimise it as a loop). So I can't comment on the performance impact of what we're doing with Array.from+Array#reduce in this PR, and whether it matters at all. :)
|
Whoops, I think we lost track of this one. Sorry about that! Pretty sure this is still an issue in trunk, @vdwijngaert if you're still interested let's rebase the PR to fix the conflict and we can try landing this one. If you're busy, I can also rebase and land for you. |
|
As a result of the bug scrub in the editor chat, I will continue to test. |
Co-authored-by: Miguel Fonseca <[email protected]>
Everything looks good to me 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes an issue where
<!--more-->comments did not keep their location when inside a paragraph, after being converted to blocks. This mostly affects those cases where a classic editor block gets converted. See the linked issue for details.My code addresses the special case where the
<!--more-->was placed inside of a paragraph. In order to obtain the expected behavior, we split the paragraph in two and insert the block in the middle instead.Closes #18079
Checklist:
*.native.jsfiles for terms that need renaming or removal). (not applicable)