Skip to content
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

Raw handling: Skip shortcode if not on its own line #19059

Merged
merged 1 commit into from Dec 12, 2019

Conversation

@mcsf
Copy link
Contributor

mcsf commented Dec 11, 2019

Description

Improves the existing logic for detection of "inline shortcodes" by ensuring that a shortcodes stands not only at the start of a line/paragraph, but also at the end of one.

Example

The following classic input:

[foo]A[/foo] [foo]B[/foo]

yields:

Before After
Shortcode block for [foo]A[/foo] followed by Paragraph block with content [foo]B[/foo]. Paragraph block with content equal to input: [foo]A[/foo] [foo]B[/foo].

How has this been tested?

Addition of integration tests.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.
Improves the existing logic for detection of "inline shortcodes" by
ensuring that a shortcodes stands not only at the start of a
line/paragraph, but also at the end of one.
! /(\n|<p>)\s*$/.test( beforeHTML )
! (
/(\n|<p>)\s*$/.test( beforeHTML ) &&
/^\s*(\n|<\/p>)/.test( afterHTML )

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 11, 2019

Member

This could be non capturing?

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Dec 11, 2019

Any issues that this fixes?

mcsf added a commit that referenced this pull request Dec 11, 2019
@mcsf

This comment has been minimized.

Copy link
Contributor Author

mcsf commented Dec 11, 2019

Any issues that this fixes?

Nothing tracked to my knowledge — just something I stumbled upon.


// If the shortcode content does not contain HTML and the shortcode is
// not on a new line (or in paragraph from Markdown converter),
// consider the shortcode as inline text, and thus skip conversion for
// this segment.
if (
! includes( match.shortcode.content || '', '<' ) &&
! /(\n|<p>)\s*$/.test( beforeHTML )
! (
/(\n|<p>)\s*$/.test( beforeHTML ) &&

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 11, 2019

Member

Minor: Maybe here too?

This comment has been minimized.

Copy link
@mcsf

mcsf Dec 12, 2019

Author Contributor

Actually, on second thought I'm going to roll back on the non-capturing group changes. I'm thinking that the fact that we're testing with RegExp::test instead of ::exec or String::match should be enough — and if not, that's something for the VMs to work on!

Quoting from RegExp::test:

Use test() whenever you want to know whether a pattern is found in a string. test() returns a boolean, unlike the String.prototype.search() method, which returns the index (or -1 if not found). To get more information (but with slower execution), use the exec() method (similar to the String.prototype.match() method). As with exec() (or in combination with it), test() called multiple times on the same global regular expression instance will advance past the previous match.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 12, 2019

Member

Sounds good! Thanks for looking into it! :)

@mcsf mcsf force-pushed the fix/shortcode-inline-detection branch from 96ba092 to c4b361e Dec 12, 2019
@mcsf mcsf merged commit da6d2c2 into master Dec 12, 2019
3 checks passed
3 checks passed
pull-request-automation
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@mcsf mcsf deleted the fix/shortcode-inline-detection branch Dec 12, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.