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

Parser: Parse <!-- more --> tag and <!-- noteaser --> #1460

Merged
merged 5 commits into from
Jul 4, 2017

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Jun 26, 2017

@see #1329 #983 #1440

These tags are officially supported by WordPress and are an exception
with the concept of blocks. They contain structural data and yet they
are just HTML comments without any further indication.

The "more" tag supports an optional custom text string and if it is
immediately followed by the "no teaser" tag then that contains
additional information.

In this change we parse those tags and combine them into a single "more"
block with attributes of the custom text and whether or not the
noTeaser option has been selected.

By the way, I haven't added this yet to the PHP parser. That's something I'm
still thinking about. Please feel free to augment it on this PR.

At first I planned on making this a syntax-sugar for blocks but then I realized
it's a special case and there are few other exceptions, so I made it specific
and encoded in the semantics of the noteaser tag with it.

cc: @swissspidy

@dmsnell dmsnell added the [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f label Jun 26, 2017
} }

WP_Tag_NoTeaser
= "<!--" WS* "noteaser" WS* "-->"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<!--noteaser--> support in WordPress is hardcoded to exactly this string. No whitespace allowed. See get_the_content().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I removed the allowance in b5fb0c1

/ WP_Block_Balanced
/ WP_Block_Html

WP_Tag_More
= "<!--" WS* "more" customText:(WS+ text:$((!(WS* "-->") .)+) { return text })? WS* "-->" noTeaser:(WS* WP_Tag_NoTeaser)?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #1329, <!--noteaser--> doesn't need to directly follow the more tag (although it makes sense to put it there). It can theoretically be anywhere in the post content. See get_the_content().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I deferred to the Codex for the behavior. which should we fix? the Codex or Core?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I think I can say that I would prefer we enforce the documented syntax. we're already making a break in some fashion with the old, plus when someone loads a post with an incorrect <!--noteaser--> tag it will just disappear I think, then they can see the option on the more block.

just musing thoughts here

@dmsnell
Copy link
Member Author

dmsnell commented Jun 26, 2017

This PR is not tracking the branch. I'm going to give GitHub some time in case their systems are the problem. If it doesn't resolve I'll recreate the PR.

@swissspidy
Copy link
Member

@dmsnell Yeah, it's GitHub. Just noticed it before as well. See https://status.github.com

By the way, thanks for creating this PR! I was looking into the grammar thing as well, but didn't really have time for it.

@mtias
Copy link
Member

mtias commented Jun 26, 2017

Here's a broader question: should we understand the legacy ones but always convert to new ones on save, and update core to understand new ones?

@mtias
Copy link
Member

mtias commented Jun 26, 2017

It could become:

<!-- wp:core/more { "customText": "Keep on reading!", "noTeaser": true } /-->

post-content.js Outdated
@@ -12,6 +12,9 @@ window._wpGutenbergPost = {
'<section className="cover-image wp-block-cover-image" style={ { backgroundImage: \'url("https://cldup.com/GCwahb3aOb.jpg");\' } }><h2>Gutenberg Editor</h2></section>',
'<!-- /wp:core/cover-image -->',

'<!-- more Keep on reading! -->',
'<!-- noteaser -->',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep this one out of the demo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<!-- wp:core/more { "customText": "Keep on reading!", "noTeaser": true } /-->

good question! if we do this then we do have one more unique feature: the "fallback HTML" no longer carries the same rendering because WP will not appropriate handle the more tag.

as a serialized block it will only work if we have a block registered.

I'm not sure how I feel, but I have a commit ready to serialize the normal more tag

@mtias
Copy link
Member

mtias commented Jun 26, 2017

Looks good, thanks!

@dmsnell
Copy link
Member Author

dmsnell commented Jul 3, 2017

For now, in follow-up with @swissspidy and @mtias what I'd like to do is merge this incremental change: follow the WP spec, which says that the noteaser must immediately follow the more and also withhold from trying to convert to a formal block spec on save.

Both of these questions are valid I think but could easily be done in another PR later.

These tags are officially supported by WordPress and are an exception
with the concept of blocks. They contain structural data and yet they
are just HTML comments without any further indication.

The "more" tag supports an optional custom text string and if it is
immediately followed by the "no teaser" tag then that contains
additional information.

In this change we parse those tags and combine them into a single "more"
block with attributes of the custom text and whether or not the
`noTeaser` option has been selected.
@dmsnell
Copy link
Member Author

dmsnell commented Jul 3, 2017

Rebased: former HEAD was a1343a4

@swissspidy
Copy link
Member

See also #1467 for some tests that perhaps could be useful for this. The parser still has some problems with these tags I think (see 35ef9f12a595168b14062a9377d064c163306803)

@dmsnell
Copy link
Member Author

dmsnell commented Jul 3, 2017

@swissspidy the problems with the parser in #1467 are related to the way that early on the concept of a Token was stripped from the parser. It has made it hard to deal with things since then and since we've built the parser around this weakened state. It's been on my todo list to fix but I have very little time to dedicate to it.

Therefore the problem isn't with these tags but with the WP_Block_HTML rule (which I think is a weak rule anyway to have since it's literally the absence of rules) and how it has to prevent detecting everything else that could be a top-level token.

@swissspidy
Copy link
Member

@dmsnell Yeah this WP_Block_HTML rule was just pointed out to me as well, see #1467 (comment)

@dmsnell dmsnell merged commit fdbfc9b into master Jul 4, 2017
@dmsnell dmsnell deleted the parser/parse-more-tag branch July 4, 2017 14:19
@@ -115,6 +115,10 @@ export function serializeBlock( block ) {
const saveContent = getSaveContent( blockType, block.attributes );
const saveAttributes = getCommentAttributes( block.attributes, parseBlockAttributes( saveContent, blockType ) );

if ( 'wp:core/more' === blockName ) {
return `<!-- more ${ saveAttributes.customText ? `${ saveAttributes.customText } ` : '' }-->${ saveAttributes.noTeaser ? '\n<!--noteaser-->' : '' }`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually be <!--more instead of <!-- more according to get_the_content().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #1440.

@nylen
Copy link
Member

nylen commented Jul 5, 2017

Please do not merge anything else that touches parsing without test cases in place first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants