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 inline shortcodes #6329

Merged
merged 2 commits into from Apr 27, 2018

Conversation

Projects
None yet
4 participants
@iseulde
Member

iseulde commented Apr 20, 2018

Description

This PR implements #3806 (comment).

Fixes #3806.

Only if the shortcode is preceded by a newline (or a plain paragraph tag from the Markdown converter), we should create a shortcode block. Otherwise we should like the shortcode alone as text.

I think this may be a bit too simplistic, but generally goes in the right direction. Maybe we should also check if the shortcode contains any HTML. If it does, maybe it should be "preserved" in a block.

How has this been tested?

See #3806 (comment).

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@iseulde

This comment has been minimized.

Member

iseulde commented Apr 20, 2018

Also wondering if this should only happen if the transformer is added by the shortcode block. If there is any other block transforming the shortcode, we should probably just let it happens. I think the problem here is the catch-all. Cc @mcsf.

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 20, 2018

Maybe we come back here to something like setUnknownShortcodeHandlerName?

@mcsf

This comment has been minimized.

Contributor

mcsf commented Apr 23, 2018

Also wondering if this should only happen if the transformer is added by the shortcode block. If there is any other block transforming the shortcode, we should probably just let it happens. I think the problem here is the catch-all.

Good question, though I'm not quite convinced. I guess this specific handling of inline shortcodes is mainly in place for backwards compatibility, but also as a way to cope with current limitations of the block paradigm:

  1. The goal, foremost, is to prevent breakage of inline flow when converting classic content to blocks. Even when dealing with new content, it's expectable that publishers may need to rely on "legacy" inline shortcodes for certain kinds of data or control (something that perhaps inline tokens will address in the future).

  2. Well before Gutenberg, shortcodes have been employed both inline and as "blocks" (on their own line), but I don't see that choice as necessarily bound to the shortcode type. That is, I can see myself using certain shortcodes in both ways:

Inline Block
Let's meet at [time]Tuesday, December 12, 2017 at 14:00 UTC[/time].

Meeting schedule



[time]Tuesday, December 12, 2017 at 14:00 UTC[/time]

And, conceptually, when transitioning to Gutenberg, I'd expect the latter scenario to convert to wp:heading + wp:shortcode, but the former to remain wp:paragraph.

Does this make sense, or was I just rambling? 🙂

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 23, 2018

@mcsf That makes sense. So what do you think of the handling in this PR?

@iseulde iseulde changed the title from Skip inline shortcodes to Raw handling: skip inline shortcodes Apr 23, 2018

@iseulde iseulde requested a review from WordPress/gutenberg-core Apr 23, 2018

@aduth

This comment has been minimized.

Member

aduth commented Apr 24, 2018

(or a plain paragraph tag from the Markdown converter)

Should the shortcode converter have knowledge of the Markdown converter? Can the shortcode conversion take place before Markdown-ification takes effect?

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 25, 2018

@aduth @mcsf We discussed this a few times, and I'm not sure what the right solution here is. Shortcakes and Markdown can conflict: [link](http://...). We could do a plain text check first, then make the shortcode parser ignore [] followed by (? Won't work for reference-style links though. This is the biggest issue.

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 25, 2018

Maybe we should only run one of these (markdown or shortcode) depending on the content, but I'm not sure what to look for. Shortcodes can also be found in plain text when pasting from the text editor.

@mcsf

mcsf approved these changes Apr 27, 2018

I'm sure we can fine-tune this in the future if we run into special cases. For now, this is a welcome fix for me.

lastIndex = match.index + match.content.length;
// If the shortcode content does not contain HTML and the shortcode is
// not on new line (or in paragraph from Markdown converter), consider
// the shortcode as inline text.

This comment has been minimized.

@mcsf

mcsf Apr 27, 2018

Contributor

I'd add at the end of the sentence: , and thus skip conversion for this segment. Does it make sense?

edit: I've amended

@mcsf mcsf added this to the 2.8 milestone Apr 27, 2018

@mcsf mcsf merged commit 81af13e into master Apr 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mcsf mcsf deleted the try/skip-inline-shortcode branch Apr 27, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 27, 2018

Thanks for reviewing @mcsf!

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 6, 2018

@iseulde @mcsf Is there a reason there aren't tests for this pull request (and segmentHTMLToShortcodeBlock more generally)?

@iseulde

This comment has been minimized.

Member

iseulde commented Jun 8, 2018

@danielbachhuber Yes. There's no module yet for the shortcodes, it's still a window global.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment