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

Add pagination block #1467

Merged
merged 1 commit into from Apr 9, 2018

Conversation

@swissspidy
Member

swissspidy commented Jun 26, 2017

#1440 and #1460 inspired me to add a new block to support <!--nextpage--> in Gutenberg.

See #1328.

This PR does:

  • add a new core/nextpage block
  • update special-comment-converter.js to turn <!--nexpage--> into that new block
  • add tests

Screenshots:

screen shot 2017-06-26 at 21 46 53

screen shot 2017-06-26 at 21 47 04

screen shot 2017-06-26 at 21 47 16

@chrisgrabinski

This comment has been minimized.

chrisgrabinski commented Jun 26, 2017

It might be useful to add page numbers to the block output. This would improve navigation inside the editor for long articles with multiple pages.

page-numbers

@dmsnell

This comment has been minimized.

Contributor

dmsnell commented Jun 26, 2017

@swissspidy this looks fine to me, but yeah we may want to revisit. Open questions include:

  • how many of these exceptions are there where we have a magical HTML comment?
  • should we create a new top-level rule in the parser for the magic comments? (I started this way in #1460 but took it out as a "when we get there" kind of thing)
  • where should we attempt to track the stateful information, such as which page number we're breaking? we can do it in the parser, though that's really something we want to avoid since it breaks the parsing algorithm. we can do it in the parser.js file, but we have to make sure we don't start proliferating hacks in there.

so far though this PR looks good to me. thanks!

@youknowriad

Code looks good but for some reason the serializer is not working as expected (Try switching to the text mode you'll notice the <!-- wp:core/nextpage /--> comments instead of <!--nextpage-->

@swissspidy

This comment has been minimized.

Member

swissspidy commented Jun 27, 2017

Thanks @youknowriad, I fixed the serializer now and added a test to verify that. Needed to move some declarations in order to not get warnings b/c of missing attributes.


how many of these exceptions are there where we have a magical HTML comment?

To my knowledge, there are only <!--more(.*?)?-->, <!--noteaser--> and <!--nextpage-->.

should we create a new top-level rule in the parser for the magic comments? (I started this way in #1460 but took it out as a "when we get there" kind of thing)

Since there are only 3 special HTML comments, it might make sense to have 1 specific top-level rule for them in the parser.

It might be useful to add page numbers to the block output. This would improve navigation inside the editor for long articles with multiple pages.

Good idea. I thought I saw some code in Gutenberg for such stuff the other day, but I can't find it right now.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 27, 2017

Looks like there's also an issue with the parser, try saving a post with this block and reloading the page.

@swissspidy

This comment has been minimized.

Member

swissspidy commented Jun 27, 2017

Ugh, indeed.

I just added a failing test in 35ef9f1 to demonstrate this.

The test results in only 3 blocks, where the third block gets a raw content of \n<p>Broccoli</p>\n<!--nextpage--><p>Romanesco</p> instead of just <p>Broccoli</p> and the rest being split into more blocks.

@swissspidy

This comment has been minimized.

Member

swissspidy commented Jul 2, 2017

@youknowriad Do you perhaps have an idea regarding the parser and how I could fix it?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jul 3, 2017

@swissspidy I think we'd need to exclude the WP_Block_NextPage block from the WP_Block_HTML (line 51 of the pegjs file) (same as void block for example)

@swissspidy

This comment has been minimized.

Member

swissspidy commented Jul 3, 2017

Interesting, I haven't seen that before. I did that in 8ee3f50 but it doesn't seem to work. See https://travis-ci.org/WordPress/gutenberg/jobs/249624708#L2010-L2016

@swissspidy

This comment has been minimized.

Member

swissspidy commented Jul 4, 2017

Ugh, so the tests partly just failed because of #1460. I needed to add a save() method when registering the blocks in the test.

@swissspidy swissspidy requested review from youknowriad and dmsnell Jul 4, 2017

@@ -95,7 +112,7 @@ WP_Block_Balanced
}
WP_Block_Html
= ts:(!WP_Block_Balanced !WP_Block_Void c:Any {
= ts:(!WP_Block_Balanced !WP_Block_Void !WP_Block_NextPage c:Any {

This comment has been minimized.

@dmsnell

dmsnell Jul 4, 2017

Contributor

Should probably also put in !WP_Tag_More here until we fix up the root cause of this trouble

This comment has been minimized.

@swissspidy

swissspidy Jul 4, 2017

Member

Should I also rename WP_Block_NextPage to WP_Tag_NextPage?

This comment has been minimized.

@dmsnell

dmsnell Jul 4, 2017

Contributor

It's up to you. It would be nice if they were both the same. I chose Tag because of the exceptional nature of the tag, not knowing originally it would turn into a block. I think that difference is legitimate here since it's clearly not a block in the text.

Either way, could we normalize them both to match each other?

if ( 'core/nextpage' === blockName ) {
return '<!--nextpage-->';
}

This comment has been minimized.

@dmsnell

dmsnell Jul 4, 2017

Contributor

would probably be nice to keep this next to the more tag. if it's up here to save on computation I would suggest that there's not much to save on this one special tag over the cost of serializing the document.

This comment has been minimized.

@swissspidy

swissspidy Jul 4, 2017

Member

I know it would be nice, but it's up there to not cause errors when calling getSaveContent() or getCommentAttributes() because this block doesn't have any of that.

This comment has been minimized.

@dmsnell

dmsnell Jul 4, 2017

Contributor

is that problematic then that the block doesn't do it. maybe we've surfaced an oddity or bug?

it seems like other blocks might also have no attributes and if they break here we will want to make sure that doesn't happen.

This comment has been minimized.

@swissspidy

swissspidy Jul 4, 2017

Member

Hmm, I had another look and it seems like the problem just was the missing save() method in the test, no other warnings. So I guess that resolved itself.

expect( parsed[ 0 ].attributes.content ).to.eql( '<p>Cauliflower</p>' );
expect( parsed[ 2 ].attributes.content ).to.eql( '<p>Broccoli</p>' );
expect( parsed[ 4 ].attributes.content ).to.eql( '<p>Romanesco</p>' );
} );

This comment has been minimized.

@nylen

nylen Jul 5, 2017

Member

This last test case in particular is probably a better fit for a new file in blocks/test/fixtures/, for example misc__multi-blocks-and-nextpage.html. This way it will be both easier to set up and more thoroughly tested.

This comment has been minimized.

@swissspidy

swissspidy Jul 5, 2017

Member

Sounds reasonable. Is there precedent for this or shall I just go on to create the first such misc__… file?

This comment has been minimized.

@nylen

nylen Jul 5, 2017

Member

This would be the first such fixture, yes. There need to be many more before we are ready to release.

/**
* WordPress dependencies
*/
import { __ } from 'i18n';

This comment has been minimized.

@aduth

aduth Aug 3, 2017

Member

Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:

git fetch origin
git rebase origin/master
@gziolo

This comment has been minimized.

Member

gziolo commented Jan 27, 2018

@swissspidy, do you plan to continue the work started in this PR. From the comments looks like it was in pretty good shape a few months back. It might require some extra work to make it up to date.

@mcsf

This comment has been minimized.

Contributor

mcsf commented Feb 13, 2018

@swissspidy, I'm interested in this per #5031. Let me know what the status is, or if you'd like to collaborate on it or reassign it. :)

@swissspidy

This comment has been minimized.

Member

swissspidy commented Feb 14, 2018

@mcsf Interesting! Would be happy to collaborate. I'll try to resolve conflicts this week and then we can iterate from there.

@mcsf

This comment has been minimized.

Contributor

mcsf commented Feb 20, 2018

Hi, @swissspidy, how're things here? :)

@swissspidy

This comment has been minimized.

Member

swissspidy commented Feb 20, 2018

Works like a charm again. Demo:

screen shot 2018-02-20 at 13 26 52

screen shot 2018-02-20 at 13 27 49

Adding page numbers to the block was previously mentioned, but I think that could be added in a new PR if considered useful.

@mcsf Feel free to check it out now :)

@mcsf

This comment has been minimized.

Contributor

mcsf commented Mar 1, 2018

@swissspidy, apologies for the delay.

Thanks for refreshing this branch. I wanted to have a couple of other PRs merged before getting back to you. The crux is #5061, which reworks the More block so as to not impact the parser. I'd like for the pagination block to follow the same pattern. In other words, it shouldn't add anything to the PEG parser, thus optimizing for low parser complexity, meaning better robustness, performance and separation of concerns.

A couple of comments to have in mind from that PR:

If this change is course is frustrating, I understand. If you'd still like to take this on, I'd be quite grateful!

@swissspidy

This comment has been minimized.

Member

swissspidy commented Mar 1, 2018

As mentioned in that other PR, special-comment-converter might need to undergo some changes. Apart from that this should be fine for the moment.

@mcsf Mind having another look now?

@mcsf

Thanks for the super quick response! This is looking good. I've left some comments.

return;
}
if ( node.nodeValue.indexOf( 'nextpage' ) === 0 ) {

This comment has been minimized.

@mcsf

mcsf Mar 1, 2018

Contributor

I don't believe nextpage accepts any "arguments" within the comment, so, in order to match core, this should be a full string eq check: node.nodeValue === 'nextpage'.

parent.appendChild( nextPage );
node.parentNode.removeChild( node );
return;

This comment has been minimized.

@mcsf

mcsf Mar 1, 2018

Contributor

Instead of repeating the parent finding and the node operations for both comment types, we could refactor this. The simplest way I can think of may be:

let newNode;

if ( foundNextpage ) {
  newNode = createNextpage();
} else if ( foundMore ) {
  newNode = createMore( ...args );
}

if ( newNode ) {
  let parent = …; while ( … ) parent = parent.Node;
  parent.appendChild( … ); node.parentNode.removeChild( … );
  return;
}

or, actually better:

if ( foundNextpage ) {
  appendToBody( node, createNextpage() );
  remove( node );
  return;
}
if ( foundMore ) {
  appendToBody( node, createMore( … ) );
  remove( node );
}

function appendToBody( processedNode, newNode ) {
  // Append it to the top level for later conversion to blocks
  let parent = node.parentNode;
  while ( parent.nodeName !== 'BODY' ) {
    parent = parent.parentNode;
  }

  parent.appendChild( nextPage );
}

function remove( node ) {
  node.parentNode.removeChild( node );
}
title: __( 'Page break' ),
description: __( 'This block allows you to set break points on your post. Visitors of your blog are then presented with content split into multiple pages.' ),

This comment has been minimized.

@mcsf

mcsf Mar 1, 2018

Contributor

A keyword property here would be useful to include things like next page, helping with discovery in the block inserters.

save() {
return (
<RawHTML>
{ [ '<!--nextpage-->' ].join( '\n' ) }

This comment has been minimized.

@mcsf

mcsf Mar 1, 2018

Contributor

No need for the array + join operation :)

<RawHTML>
  { '<!--nextpage-->' }
</RawHTML>
@swissspidy

This comment has been minimized.

Member

swissspidy commented Mar 2, 2018

@mcsf I've incorporated your feedback now.

At the same time I realised that appending these elements to the body totally breaks the posts. This worked way better before #5061 and is now really frustrating for users. Put simply, I'd say this is not acceptable. Especially when it can be done better. Replacing the existing nodes makes much more sense, although still doesn't catch things like <p>Foo<!--nextpage-->Bar</p>.

Here are some examples:

Page breaks before conversion:
page before

Page breaks after conversion:
page after

More tag before conversion:
more before

More tag after conversion:
more after

@swissspidy

This comment has been minimized.

Member

swissspidy commented Mar 3, 2018

Here's more or less what I had in mind for improving the node replacement:

diff --git blocks/api/raw-handling/special-comment-converter.js blocks/api/raw-handling/special-comment-converter.js
index ac2e1bc7..85fa5aad 100644
--- blocks/api/raw-handling/special-comment-converter.js
+++ blocks/api/raw-handling/special-comment-converter.js
@@ -24,8 +24,7 @@ export default function( node ) {
 	}
 
 	if ( node.nodeValue === 'nextpage' ) {
-		appendToBody( node, createNextpage() );
-		remove( node );
+		replace( node, createNextpage() );
 		return;
 	}
 
@@ -51,8 +50,7 @@ export default function( node ) {
 			}
 		}
 
-		appendToBody( node, createMore( customText, noTeaser ) );
-		remove( node );
+		replace( node, createMore( customText, noTeaser ) );
 	}
 }
 
@@ -76,14 +74,13 @@ function createNextpage() {
 	return node;
 }
 
-function appendToBody( processedNode, newNode ) {
-	// Append it to the top level for later conversion to blocks
-	let parent = processedNode.parentNode;
-	while ( parent.nodeName !== 'BODY' ) {
-		parent = parent.parentNode;
-	}
+function replace( processedNode, newNode ) {
+	insertAfter( newNode, processedNode.parentNode );
+	remove( processedNode );
+}
 
-	parent.appendChild( newNode );
+function insertAfter( newNode, referenceNode ) {
+	referenceNode.parentNode.insertBefore( newNode, referenceNode.nextSibling );
 }
 
 function remove( node ) {
@mcsf

This comment has been minimized.

Contributor

mcsf commented Mar 9, 2018

Hey, @swissspidy, apologies for the delay, and great work here.

At the same time I realised that appending these elements to the body totally breaks the posts. This worked way better before #5061 and is now really frustrating for users. Put simply, I'd say this is not acceptable. Especially when it can be done better. […]

Totally agree. I've tested your patch and it works great. I also wrote some tests for it. If you don't mind, I'll push a PR — so that we can fix More quickly — and give you props for it. I'll circle back for a review of Next Page.

@mcsf mcsf referenced this pull request Mar 9, 2018

Merged

Block conversion: Preserve "More" order #5538

0 of 3 tasks complete

@swissspidy swissspidy requested a review from mcsf Mar 27, 2018

@swissspidy

This comment has been minimized.

Member

swissspidy commented Mar 27, 2018

Rebased against master after #5538.

@mcsf Mind having another look?

@mcsf

This comment has been minimized.

Contributor

mcsf commented Mar 27, 2018

@mcsf

This looks great, thanks for stewarding and for your patience! Left some notes.

@@ -9,7 +9,7 @@ import { remove, replace } from '@wordpress/utils';
const { COMMENT_NODE } = window.Node;
/**
* Looks for `<!--more-->` comments, as well as the `<!--more Some text-->`
* Looks for `<!--nextpage-->` and `<!--more-->` comments, as well as the `<!--more Some text-->`

This comment has been minimized.

@mcsf

mcsf Mar 30, 2018

Contributor

Minor, but this is better with reformatting for line length:

 * Looks for `<!--nextpage-->` and `<!--more-->` comments, as well as the
 * `<!--more Some text-->` variant and its `<!--noteaser-->` companion, and
 * replaces them with a custom element representing a future block.
description: __( 'This block allows you to set break points on your post. Visitors of your blog are then presented with content split into multiple pages.' ),
icon: 'admin-page',

This comment has been minimized.

@mcsf

mcsf Mar 30, 2018

Contributor

admin-page seems alright to me, but if @jasmussen has some other suggestion, 🙇

This comment has been minimized.

@jasmussen

jasmussen Apr 9, 2018

Contributor

Yep, that icon is solid.

{
"blockName": "core/nextpage",
"attrs": null,
"innerBlocks": [],

This comment has been minimized.

@mcsf

mcsf Mar 30, 2018

Contributor

Indentation needs fixing here

.wp-block-nextpage > span:after {
left: 100%;
margin-left: 20px;
}

This comment has been minimized.

@mcsf

mcsf Mar 30, 2018

Contributor

This file is a lot different from blocks/library/more/editor.scss. I prefer the final result for Next Page, and we'd probably want to converge a bit on the two stylesheets. Any advice from @jasmussen is much appreciated, as always.

This comment has been minimized.

@jasmussen

jasmussen Apr 9, 2018

Contributor

I agree it would be nice to refine the styles of both of these blocks. However this PR has gone on for a while. I think perhaps it'd be good to merge this in as is, and then look at refining/unifying the styles separately. I can take that on as a quick smallish PR.

@swissspidy swissspidy requested a review from jasmussen Apr 9, 2018

@mcsf

This comment has been minimized.

Contributor

mcsf commented Apr 9, 2018

I think perhaps it'd be good to merge this in as is, and then look at refining/unifying the styles separately. I can take that on as a quick smallish PR.

@jasmussen, that'd be awesome!

@swissspidy, seems like we're good to go, then. One last thing: can you rebase this entire branch into one-to-three neat commits against the latest master? Once you've got it, I'll merge it. 🙇

@swissspidy

This comment has been minimized.

Member

swissspidy commented Apr 9, 2018

@mcsf Should look better now with just one commit.

@mcsf mcsf merged commit 909d38d into WordPress:master Apr 9, 2018

2 checks passed

codecov/project 44.61% (+0.04%) compared to 44fdc39
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mcsf

This comment has been minimized.

Contributor

mcsf commented Apr 9, 2018

This is great! Thanks for your perseverance, @swissspidy. :)

@mcsf mcsf added this to the 2.7 milestone Apr 10, 2018

jasmussen added a commit that referenced this pull request Apr 12, 2018

Unify markup between More block and Pagination block
This adds a white background, and some things, to the Pagination block. This will only be visible if a user loads a stylesheet into the editor.

Fixes #1467 (comment), cc @swissspidy.

jasmussen added a commit that referenced this pull request Apr 12, 2018

Unify markup between More block and Pagination block
This adds a white background, and some things, to the Pagination block. This will only be visible if a user loads a stylesheet into the editor.

Fixes #1467 (comment), cc @swissspidy.

jasmussen added a commit that referenced this pull request Apr 12, 2018

Add polish (#6138)
* Polish link dialog.

This uses variables for colors and shadows, and also fixes a regression with the link dialog when linking an image.

However there's still a bug here, where as soon as you start typing in the image link dialog, the entire thing disappears and you can't type. This is possibly due to isEditing mode being invoked there. Can you take a look, @noisysocks @karmatosed?

* Unify markup between More block and Pagination block

This adds a white background, and some things, to the Pagination block. This will only be visible if a user loads a stylesheet into the editor.

Fixes #1467 (comment), cc @swissspidy.

* Unify button sizes, and slim down "Media Library".

This makes the placeholders fit better in small breakpoints.

* Change left arrow on breadcrumbs.

This makes the left arrow consistent with the one used when creating links on images.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment