query-pagination-next and previous: Changing the markup on the first and last pages of a pagination #36681

matiasbenedetto · 2021-11-19T18:55:27Z

Hi 👋

Description

This PR changes the rendering of core/query-pagination-next and core/query-pagination-previous for 2 "edge cases". These cases are when the user is navigating the first or last page of a paginated query. In these cases, we render a <span> instead of nothing as we commonly do with the pagination numbers where the number matching the pagination's current number renders a <span> instead of an <a>.

After the changes featured in this PR the expected behaviour is:

While navigating a first page of a pagination:
- core/query-pagination-previous renders a <span> element instead of nothing.
- core/query-pagination-next renders an <a> element as always.
While navigating a last page of a pagination:
- core/query-pagination-previous renders an <a> element as always.
- core/query-pagination-next renders a <span> element instead of nothing.
While navigating any page of pagination that is not the first or the last one:
- core/query-pagination-previous renders an <a> element as always.
- core/query-pagination-next renders an <a> element as always.
While navigating a page with a core/query-pagination block in place but no data to paginate:
- Nothing is rendered as always.

This is just a continuation of a work started by @MaggieCabrera

Why are we introducing this change?

The intention is to fix the problems around the use of space-around justification in the flex container of the core/query-pagination block as described in this issue (#34997).

How has this been tested?

Tested with normal and inherit wp queries using Empty, TT1, and Blockbase themes.

Block code:

<!-- wp:query-pagination {"layout":{"type":"flex","justifyContent":"space-between"}} -->
	<!-- wp:query-pagination-previous /-->
	<!-- wp:query-pagination-numbers /-->
	<!-- wp:query-pagination-next /-->
<!-- /wp:query-pagination -->

Screenshots

First page (Empty theme):

Before	After

Last page (Empty theme):

Before	After

First page (TT1 theme):

Before	After

Last page (TT1 theme):

Before	After

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
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 (please manually search all *.native.js files for terms that need renaming or removal).

github-actions · 2021-11-19T18:55:48Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @matiasbenedetto! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

scruffian · 2021-11-19T21:41:58Z

This is working well for me but it would be good to have some designers eyes on it - I'm not sure about the Previous/Next Page text appearing when it doesn't do anything. cc @jasmussen @kjellr

scruffian · 2021-11-19T21:43:49Z

packages/block-library/src/query-pagination-next/index.php

@@ -26,7 +27,13 @@ function render_block_core_query_pagination_next( $attributes, $content, $block
 	if ( $pagination_arrow ) {
 		$label .= $pagination_arrow;
 	}
-	$content = '';
+
+	$wrapper_attributes = get_block_wrapper_attributes();


This is already defined on line 23, do we need to call it again?

scruffian · 2021-11-19T21:45:08Z

packages/block-library/src/query-pagination-next/index.php

-	$content = '';
+
+	$wrapper_attributes = get_block_wrapper_attributes();
+	$content            = sprintf(


Should we escape the output here incase the user tries to insert something untrustworthy in the attribute? (https://developer.wordpress.org/themes/theme-security/data-sanitization-escaping/)

Given that its not escaped elsewhere in this file it's probably safe...

Yep, I think so

scruffian · 2021-11-19T21:47:02Z

packages/block-library/src/query-pagination-previous/index.php

-	} elseif ( 1 !== $page ) {
-		$content = sprintf(
+	} else {
+		$content = 1 !== $page ? sprintf(


I think this goes against the brace style in the PHP guidelines: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/

jasmussen · 2021-11-22T07:59:47Z

I'm not sure about the Previous/Next Page text appearing when it doesn't do anything

I would agree, the text makes it sound actionable when it isn't. I do recall Kjell having some trouble with aligning the three harmoniously in a container, and I imagine there might be a layout challenge if the 3rd one is removed entirely.

MaggieCabrera · 2021-11-22T08:17:44Z

I'm not sure about the Previous/Next Page text appearing when it doesn't do anything

I would agree, the text makes it sound actionable when it isn't. I do recall Kjell having some trouble with aligning the three harmoniously in a container, and I imagine there might be a layout challenge if the 3rd one is removed entirely.

I think it would make sense to have it with lowered opacity. Right now it's already a challenge to keep the 3 items in the same place with any given layout if one of them disappears completely on first/last page too. And with the class added here, theme developers have the chance to style those cases in their own way too.

kjellr · 2021-11-22T16:03:37Z

Yeah, having the text present but unclickable seems incorrect here, even if it's a lower opacity. The standard behavior around the internet is basically what we have already today: the text only shows up if it's possible to paginate in that direction.

The issue we're trying to solve for here is just that we can't automatically center the page numbers if there are two items in this container. That's just not the way that space-around works. If these were standard blocks and I always wanted the page numbers centered, I might try using a columns block. Have we tried something like that here?

MaggieCabrera · 2021-11-22T16:06:01Z

The issue we're trying to solve for here is just that we can't automatically center the page numbers if there are two items in this container. That's just not the way that space-around works. If these were standard blocks and I always wanted the page numbers centered, I might try using a columns block. Have we tried something like that here?

The problem is that inside the pagination block only the query previous/next/numbers blocks are allowed, so you can't wrap them inside column blocks.

kjellr · 2021-11-22T16:08:14Z

Right, but that's theoretically something we could change if it is necessary.

scruffian · 2021-11-22T16:59:39Z

What if we use CSS grid?

kjellr · 2021-11-22T17:04:46Z

Yeah, this seems like an appropriate use case for grid. Not sure the feasibility of adding that in here though. @jasmussen are you up to date on the explorations around Grid in the editor?

jasmussen · 2021-11-22T17:17:49Z

Grid in terms of theme.json integration with corresponding UI is a bit backburnered and not something to consider. However it looks like the "Query Pagination" block (wp-block-query-pagination) comes with its own style.scss file which we could add some grid rules to. If those grid rules are sufficiently generic and utilitarian, that seems fine?

matiasbenedetto · 2021-11-23T14:33:52Z

I think using display:grid is not the best option here. Query pagination has flex controls that work fine and are useful to customize the block rendering.

What do you think about adding visibility:hidden and aria-hidden="true" to the non clickeable previous and next elements. I think that solution covers the three issue discussed here:

Solves the positioning issue
Invisible to the eye
Invisible for the screen readers

What do you think?

matiasbenedetto · 2021-11-23T15:03:11Z

I implemented the change suggested in my last comment with this commit:

Results:

First page:

Last page:

scruffian · 2021-11-23T15:08:08Z

I like this solution. It would be good to get some input from someone from an accessibility perspective, although I'm not sure who to ask...

MaggieCabrera · 2021-11-23T15:51:43Z

What happens with this solution when you align the items to the left or to the right instead of using space between?

matiasbenedetto · 2021-11-23T17:51:45Z

This last case mentioned by @MaggieCabrera (the use of justify:flex-start), can be problematic on the first page. Because the element, even though is not visible (visibility:hidden), is still there. The same happens if we use justify:flex-end and navigate the last page.

scruffian · 2021-11-24T12:27:20Z

I think using display:grid is not the best option here. Query pagination has flex controls that work fine and are useful to customize the block rendering.

We could conisder mapping the flex controls to CSS grid styles without making the user aware that's what's happening...

matiasbenedetto · 2021-11-24T15:26:28Z

I think that the latest 2 commits fix the remaining problem around left and right Flex justifications, so I think it covers all the issues discussed so far:

Solves the positioning issue with space-around
Invisible to the eye
Invisible for the screen readers
Works with left and right justified elements

Justify left on first page:

Justify left on middle page:

Justify left on last page:

Space around on first page:

Space around on middle page:

Space around on last page:

matiasbenedetto · 2021-11-29T18:01:12Z

The latest commits are fixing some failing use cases, as the last one spotted by @scruffian.
The block should render as the examples shared in my previous comment.

scruffian · 2021-12-03T09:37:21Z

I no longer see the empty span on the first and last pages...

scruffian · 2021-12-03T10:05:15Z

I no longer see the empty span on the first and last pages...

This was an environment issue!

scruffian

This is looking good to me. There's some duplication between the blocks that would be good to clear up in a followup PR.

ntsekouras · 2021-12-03T15:19:50Z

lib/block-supports/layout.php

+	$id              = uniqid();
+	$style           = gutenberg_get_layout_style( ".wp-container-$id", $used_layout, $has_block_gap_support );
+	$container_class = 'wp-container-' . $id . ' ';
+	$justify_class   = $used_layout['justifyContent'] ? 'wp-justify-' . $used_layout['justifyContent'] . ' ' : '';


Why we added this to the layout abstraction? Was something specific to pagination blocks?

Yep, we needed to identify with a class the justification of the block, without this change that was not possible.

There shouldn't be any block logic leaked into the layout abstraction. I'll check a bit the PR better to try to understand. That was the case recently with Navigation block as well, but we should find another way to handle specific block needs.

Ok, I see, let me submit a PR with this change you propose.

@ntsekouras a PR moving the functionality from Layout to Query Pagination code:
#37113

ntsekouras · 2021-12-03T15:40:34Z

packages/block-library/src/query-pagination/style.scss

@@ -32,7 +32,17 @@ $pagination-margin: 0.5em;
 		}
 	}

-	&.aligncenter {
-		justify-content: center;


This shouldn't be removed. It was added here: #34739 as a fix for align center.

That behavior is achieved using the centered justification:

I know, but we also support the center alignment and is needed for backwards compatibility.

@ntsekouras could you provide more context about backward compatibility related to this, please?

Sure. Even if the align:center achieves the same think with centered justification, we still support this alignment. So it cannot be removed because it has created a regression. You can check this on trunk by selecting align:center and checking the front-end.

Even if we removed the align:center option though and with a new block migration in place, we would still need to keep the css rule because the migrations run and update the blocks markup in the editor and then we have to also save to get the new markup in the post content. So for content with blocks that had align:center but the migration was never applied, the content should look as before in the front end without and broken styles.

Makes sense? Please tell me to clarify anything from the above if you want.

ntsekouras · 2021-12-03T16:14:45Z

packages/block-library/src/query-pagination-previous/index.php

-		);
+	} elseif ( ! $max_page || $max_page > $page ) {
+		$custom_query  = new WP_Query( build_query_vars_from_query_block( $block, $page ) );
+		$max_num_pages = $custom_query->max_num_pages ? $custom_query->max_num_pages : 1;


Why do we need this max_num_pages logic? Previously it would enter this if when it was a custom query and it wasn't on the first page. So wouldn't a check about $page === 1 be enough? Am I missing something?

This is necessary because if the query has no results $custom_query->max_num_pages equals to zero. So the logic to know if there are multiple pages 1 < $max_num_pages doesn't work without this.

I'm sorry but I still don't get it. In Query Pagination Previous we only care about whether we are on the first page or not, no? If we are on the first page, we don't have a link to show, but if we are not we always have a link. This is different from Query Pagination Next where we have to recreate the query run from Query Loop to know if we have a link for the next posts.

Nope, we don't care only about if we are on the first page or not. We need to know if there are pages to paginate. If there are not pages to paginate should not render anything.

…requirements. This characteristic was introduced here: WordPress#36681 and currently it is only necesary for the Query Pagination block so we can move the functionality to the block's code to avoid adding extra logic to the Layout block support.

ntsekouras · 2021-12-03T22:20:32Z

Aside my comments that I'm trying to understand the changes here, I'd really want to thank you @matiasbenedetto for your contribution and effort here! Really appreciate it that you have also opened a follow up PR 💯

ntsekouras · 2021-12-04T11:13:45Z

I've created a follow up PR to discuss/check how to simplify things (now is WIP).

ntsekouras · 2021-12-06T18:52:10Z

👋 I noticed this PR when this notice was shown:

but it made me look into the changes here. I tried to understand the added complexity, especially in the case of inherit the global query case, where the added logic seemed to me like trying to duplicate the get_next_posts_link and get_previous_posts_link. That led to a draft PR which I just closed.

Personally the concept of adding an extra element (hidden wrapper) for design purposes seems a bit odd to me and I'm not really sure if this should be added in the first place. Would another kind of layout be a better fit for these blocks (even if it doesn't exist yet like grid)? If yes, could this be something that a theme could accomplish with css overrides until it's properly implemented in core/GB?

Additionally it seems the vertical layout still renders the placeholders and that can't be desired I think.

Screen.Recording.2021-12-06.at.8.40.45.PM.mov

To sum up, I think we should consider reverting this PR and see for alternative approaches that should properly fix the design issues we want. I'm worried that if we follow this path, we just hack our way around the root issue and add complexity in block logic and styles to accommodate cases like the flex-start where we now display:none, the vertical layout etc..

The root issue for me is that using flex layout for Query Pagination might not just be the right layout for this block.

--cc
@youknowriad @mcsf @gziolo @ockham

matiasbenedetto · 2021-12-06T19:50:37Z

Personally, I don't see how using CSS grid could make this better for users and theme devs.

I haven't tested the vertical flex orientation in this component before. The fix for the vertical orientation requires a really tiny CSS change proposed in this PR.

kjellr · 2021-12-16T17:30:11Z

To sum up, I think we should consider reverting this PR and see for alternative approaches that should properly fix the design issues we want. I'm worried that if we follow this path, we just hack our way around the root issue and add complexity in block logic and styles to accommodate cases like the flex-start where we now display:none, the vertical layout etc..

The root issue for me is that using flex layout for Query Pagination might not just be the right layout for this block.

I haven't been keeping up to date on developments since this comment, but for what it's worth, I think reverting this would be fine for now. Two thoughts:

The solution here is a bit complicated, and I think we can probably come up with something more ideal with a bit more time. We're leaning on the flex layouts more and more as we continue to build themes, so we may stumble on something better soon.
The design issue this is solving is kind of awkward, but not a deal-breaker for me. The end state we reached is also a little awkward, since the homepage will still have empty space on one side.
Plus, in the immediate future, I think we can work around the design/layout issue by just removing the numbered pagination from the default display of the block. I don't think it's a must-have block by default, and folks can add it in manually if they require it.

mcsf · 2021-12-17T16:33:14Z

The solution here is a bit complicated, and I think we can probably come up with something more ideal with a bit more time. We're leaning on the flex layouts more and more as we continue to build themes, so we may stumble on something better soon.

The design issue this is solving is kind of awkward, but not a deal-breaker for me. The end state we reached is also a little awkward, since the homepage will still have empty space on one side.

Plus, in the immediate future, I think we can work around the design/layout issue by just removing the numbered pagination from the default display of the block. I don't think it's a must-have block by default, and folks can add it in manually if they require it.

Thanks for the design and product perspective! They match my assessment of the situation and give me confidence to also recommend reverting. Especially since we're dealing with a non-must-have issue that can be handled downstream. /cc @matiasbenedetto @jeffikus

scruffian · 2021-12-17T19:49:49Z

It looks like the revert button isn't going to work on this one.

matiasbenedetto requested a review from ajitbohra as a code owner November 19, 2021 18:55

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 19, 2021

scruffian reviewed Nov 19, 2021

View reviewed changes

MaggieCabrera requested review from jffng, kjellr and carolinan November 22, 2021 08:18

MaggieCabrera added the [Block] Query Pagination Affects the Query Pagination Block - used for pagination within the Query Loop Block label Nov 22, 2021

matiasbenedetto requested a review from scruffian November 23, 2021 15:04

matiasbenedetto force-pushed the fix/34997 branch from a86850d to 257760b Compare November 24, 2021 14:29

matiasbenedetto requested review from spacedmonkey and TimothyBJacobs as code owners November 24, 2021 15:25

matiasbenedetto force-pushed the fix/34997 branch from e7d88c5 to 64fe5b9 Compare November 29, 2021 17:45

matiasbenedetto added 2 commits November 29, 2021 12:50

fixing css selectors

760873d

removing debugging line

844d381

scruffian approved these changes Dec 3, 2021

View reviewed changes

scruffian merged commit 18214af into WordPress:trunk Dec 3, 2021

github-actions bot added this to the Gutenberg 12.2 milestone Dec 3, 2021

matiasbenedetto mentioned this pull request Dec 3, 2021

Blockbase: removing fixed grid styles from the query-pagination Automattic/themes#5141

Closed

ntsekouras reviewed Dec 3, 2021

View reviewed changes

matiasbenedetto mentioned this pull request Dec 3, 2021

Avoids polluting Layout functionality with Query Pagination specific requirements. #37113

Closed

6 tasks

ntsekouras mentioned this pull request Dec 4, 2021

[Block Library - Query Pagination] Simplify pagination blocks #37125

Closed

3 tasks

youknowriad mentioned this pull request Dec 6, 2021

Fix Navigation accessibility issues #36292

Merged

6 tasks

ntsekouras mentioned this pull request Dec 6, 2021

make sure justifyContent exists before using it #37148

Closed

7 tasks

This was referenced Dec 20, 2021

Try cascading nav styles through classnames #37473

Merged

Revert Query Pagination Next/Previous placeholder(36681) #37520

Merged

pbking mentioned this pull request Apr 22, 2022

Render hidden span when on first/last page #40553

Closed

ntsekouras mentioned this pull request May 5, 2022

[Block Library - Query Pagination]: Try rendering hidden element when no next/previous link exists #40851

Closed

scruffian mentioned this pull request Jul 28, 2022

Query Pagination: Correctly position the "next" link on the first page. #42764

Merged

query-pagination-next and previous: Changing the markup on the first and last pages of a pagination #36681

query-pagination-next and previous: Changing the markup on the first and last pages of a pagination #36681

Conversation

matiasbenedetto commented Nov 19, 2021 • edited Loading

Description

Why are we introducing this change?

How has this been tested?

Screenshots

Types of changes

Checklist:

github-actions bot commented Nov 19, 2021

scruffian commented Nov 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Nov 22, 2021

MaggieCabrera commented Nov 22, 2021

kjellr commented Nov 22, 2021

MaggieCabrera commented Nov 22, 2021

kjellr commented Nov 22, 2021

scruffian commented Nov 22, 2021

kjellr commented Nov 22, 2021

jasmussen commented Nov 22, 2021 • edited Loading

matiasbenedetto commented Nov 23, 2021

matiasbenedetto commented Nov 23, 2021 • edited Loading

scruffian commented Nov 23, 2021

MaggieCabrera commented Nov 23, 2021

matiasbenedetto commented Nov 23, 2021 • edited Loading

scruffian commented Nov 24, 2021

matiasbenedetto commented Nov 24, 2021 • edited Loading

matiasbenedetto commented Nov 29, 2021 • edited Loading

scruffian commented Dec 3, 2021

scruffian commented Dec 3, 2021

scruffian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiasbenedetto Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiasbenedetto Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiasbenedetto Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntsekouras commented Dec 3, 2021

ntsekouras commented Dec 4, 2021 • edited Loading

ntsekouras commented Dec 6, 2021 • edited Loading

matiasbenedetto commented Dec 6, 2021 • edited Loading

kjellr commented Dec 16, 2021 • edited Loading

mcsf commented Dec 17, 2021

scruffian commented Dec 17, 2021

matiasbenedetto commented Nov 19, 2021 •

edited

Loading

jasmussen commented Nov 22, 2021 •

edited

Loading

matiasbenedetto commented Nov 23, 2021 •

edited

Loading

matiasbenedetto commented Nov 23, 2021 •

edited

Loading

matiasbenedetto commented Nov 24, 2021 •

edited

Loading

matiasbenedetto commented Nov 29, 2021 •

edited

Loading

matiasbenedetto Dec 3, 2021 •

edited

Loading

matiasbenedetto Dec 3, 2021 •

edited

Loading

matiasbenedetto Dec 3, 2021 •

edited

Loading

ntsekouras commented Dec 4, 2021 •

edited

Loading

ntsekouras commented Dec 6, 2021 •

edited

Loading

matiasbenedetto commented Dec 6, 2021 •

edited

Loading

kjellr commented Dec 16, 2021 •

edited

Loading