-
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
Properly convert successive shortcodes #7846
Conversation
To debug the test case, add a When you do so, you'll notice that But why is Adding another Take a look at the last string instance output: The last instance is different than its peers in that: if you remove the shortcode, then the remaining HTML is simply a I don't have any good ideas on how to unwind this issue though. |
77fb88d
to
ce4dfb7
Compare
Chatted about this with @danielbachhuber , seems to stem from this related Core issue: https://core.trac.wordpress.org/ticket/35545 Which is that we're using shortcode regular expressions as stateful ( There's not much to go by so far as the context for memoizing in the initial introduction of the My intuition is: Remove |
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.
Looks good 👍
packages/shortcode/src/index.js
Outdated
@@ -143,9 +143,9 @@ export function string( options ) { | |||
* | |||
* @return {RegExp} Shortcode RegExp. | |||
*/ | |||
export const regexp = memize( ( tag ) => { | |||
export const regexp = ( tag ) => { |
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: Simply for consistency, we could format this as the other plain functions in this file now:
export function regexp( tag ) {
// ...
}
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.
Simply for consistency, we could format this as the other plain functions in this file now:
Good point, 1bf5ba0
packages/shortcode/src/index.js
Outdated
return new RegExp( '\\[(\\[?)(' + tag + ')(?![\\w-])([^\\]\\/]*(?:\\/(?!\\])[^\\]\\/]*)*?)(?:(\\/)\\]|\\](?:([^\\[]*(?:\\[(?!\\/\\2\\])[^\\[]*)*)(\\[\\/\\2\\]))?)(\\]?)', 'g' ); | ||
} ); | ||
}; |
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.
Semi-colon should be removed.
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 wouldn't hate you if you just removed it yourself :) a712bf7
Merging to incorporate into #7892. The failure is a codecov false positive. |
Does it also handle nesting?
Would it create 2 blocks? |
There seems to be issues still with converting blocks that are next to each other. I recorded a quick video to show you: https://a.cl.ly/jkuOjvOQ There are many page builders out there that are shortcode based and basically everything added to the page are shortcodes and there isn't any space between them. So there is no way currently to convert these page builders to Blocks without having to first manually space out the shortcodes. Thanks for all the great works you guys do over there! |
Description
Removes
memize()
call aroundwp.shortcode.regexp()
to avoid buggy behavior when converting successive shortcodes.Fixes #7030
How has this been tested?
Types of changes
Bug fix.
Checklist: