-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Block conversion: Preserve "More" order #5538
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
Conversation
* Props @swissspidy * Properly replace nodes in rawHandler * Extend tests for rawHandler's `special-comment-converter` transform * Add rawHandler integration test `classic`
| return node; | ||
| } | ||
|
|
||
| function replace( processedNode, newNode ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Worth adding these to dom utils? We're doing these in a few places I think, even outside the paste component. Could be done separately though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To do: actually refactor elsewhere in the code base.
| @@ -0,0 +1,4 @@ | |||
| <p>First paragraph.</p> | |||
| <p><!--more--></p> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth adding more tests here such as <p>First <!--more--> paragraph.</p>?
|
|
||
| const types = [ | ||
| 'plain', | ||
| 'classic', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could consolidate the other image and iframe test into one "classic" conversion test similar to the other integration tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like that, or something along those lines. Better left to another PR, though, no?
| return `<p>${ string }</p>`; | ||
| } | ||
|
|
||
| function withEmptyP( string ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is any create than without this function. withEmptyP leases me wondering where exactly that empty paragraph will be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed; removing simplifies things.
| output.substr( start ), | ||
| '<wp-block data-block="core/more" data-no-teaser=""></wp-block>' | ||
| trimWhitespace( output ), | ||
| trimWhitespace( `<p>First paragraph.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why trimWhitespace is necessary. Shouldn't it be preserved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I mean, isn't specialCommentConverter merely moving other nodes?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. As I was developing I wanted the tests to have these input/output strings neatly aligned with the rest of the code but that meant adding tab characters throughout. Not that that's wrong, but it made the tests more brittle. I'll probably remove this.
|
Thanks for the review, @iseulde. Mind having one last look? |
bd5cf9d to
9874d43
Compare
aduth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Fixes the issue described by @swissspidy in #1467 (comment)
Kudos @swissspidy for the fix; I merely added tests.
(drive-by pull request; can provide further details later)
Description
How Has This Been Tested?
Screenshots (jpeg or gifs if applicable):
Types of changes
Checklist: