-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Allow pasting shortcodes in blocks with content. #3883
Allow pasting shortcodes in blocks with content. #3883
Conversation
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 recommend adding tests for this, as there's a lot going on during pasting in general. :)
blocks/api/raw-handling/index.js
Outdated
@@ -57,8 +57,14 @@ export default function rawHandler( { HTML, plainText = '', mode = 'AUTO' } ) { | |||
HTML = converter.makeHtml( plainText ); | |||
} | |||
|
|||
const shortcodesConverted = shortcodeConverter( HTML ); |
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.
The morphology (esp. the ending in an adjective) irks me a bit. Perhaps renaming and adding a comment:
// An array of HTML strings and block objects. The blocks replace matched shortcodes.
const pieces = …;
An alternative could be the quite awkward htmlAndBlocks
. :)
blocks/api/raw-handling/index.js
Outdated
const shortcodesConverted = shortcodeConverter( HTML ); | ||
|
||
//if the shortcodeConverter returns just one element it has no shortcodes | ||
//even if we paste only one shortcode it returns more than one element because empty strings are added |
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.
Comment format should be:
// If the …
// even if
i.e., capitalized sentences and a leading space for each line. Bonus if lines are limited in length (my preference is 78/80 chars, but commonly we see 100 as a hard limit).
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.
As for the comment itself, I'd make it terser, e.g. shortcodeConverter will always return more than one element if shortcodes are matched, as empty HTML strings are included.
blocks/api/raw-handling/index.js
Outdated
|
||
//if the shortcodeConverter returns just one element it has no shortcodes | ||
//even if we paste only one shortcode it returns more than one element because empty strings are added | ||
const hasShortCodes = shortcodesConverted.length > 1; |
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.
Spelling: hasShortcodes
blocks/api/raw-handling/index.js
Outdated
// Return filtered HTML if it's inline paste or all content is inline. | ||
if ( mode === 'INLINE' || ( mode === 'AUTO' && isInlineContent( HTML ) ) ) { | ||
if ( ! hasShortCodes && ( mode === 'INLINE' || ( mode === 'AUTO' && isInlineContent( HTML ) ) ) ) { |
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.
Perhaps refactoring like so:
const isInline = mode === 'INLINE' || ( mode === 'AUTO' && isInlineContent( HTML ) );
const hasShortcodes = …;
if ( ! hasShortcodes && isInline ) {
Referencing related issue #3806. We can probably go ahead with this fix, but the logic in |
a726511
to
57d16de
Compare
57d16de
to
02dade4
Compare
Hi @mcsf, thank you for your review I tried to address the points you raised. Regarding the tests, it looks like we don't have yet tests for the shortcode parsing mechanism. That mechanism uses window.wp.shortcode, which is not available during test execution. I'm not sure what is the best way to add test cases to it probably we will need to use some mocks. |
Yeah, I think it’s fine to mock it specifically for these tests.
|
I won't block this PR because of the tests, but we should get them in. |
Description
Until now when pasting a shortcode in existing blocks if the text we paste is plain text the inline pasting mechanism takes place and the shortcode is not parsed and the text is pasted as normal plain text. If the text contains markup the shortcode is parsed.
So if we paste [gallery ids="ID"] in a paragraph with content the text is appended to the paragraph (If copying from here span's are added so this does not happen).
If we paste
<div>[gallery ids="ID"]</div>
in a paragraph with content the shortcode is parsed and an equivalent block is added.This change makes sure inline pasting mechanism does not take place if pasting shortcodes.
How Has This Been Tested?
Add some content to a paragraph or list or heading paste a shortcode verify it is correctly parsed and a block equivalent to the shortcode is added.
Try paste other content besides shortcodes and verify things work as before.
Screenshots (jpeg or gifs if applicable):
Before:
After: