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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 16 commits into from
Dec 3, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e7fb180
add an empty wrapper when at first/last page
MaggieCabrera Oct 7, 2021
f362d1f
keep label on placeholder, add extra class
MaggieCabrera Oct 15, 2021
3a89020
Query pagination previous link: Avoids printing the link if there are…
matiasbenedetto Nov 19, 2021
9218edc
Query pagination next: removing custom 'last-page' class.
matiasbenedetto Nov 19, 2021
e1e6e7a
php code style format
matiasbenedetto Nov 19, 2021
d786609
removing repeated line of code
matiasbenedetto Nov 23, 2021
5296c32
php code style
matiasbenedetto Nov 23, 2021
2c89ae6
Query pagination previous and next: non clickeable elements are hidden
matiasbenedetto Nov 23, 2021
6084c04
php lint
matiasbenedetto Nov 23, 2021
02f3c2b
Adding a class to the wrapper layout element rendered by Gutenberg
matiasbenedetto Nov 24, 2021
dde160d
Query pagination: hiding non visible elements when the justification …
matiasbenedetto Nov 24, 2021
cadb2a6
avoid printing class if is a value is missing
matiasbenedetto Nov 24, 2021
a7c4481
Fixing elements rendered on a few edge cases
matiasbenedetto Nov 29, 2021
64fe5b9
removing not needed CSS
matiasbenedetto Nov 29, 2021
760873d
fixing css selectors
matiasbenedetto Nov 29, 2021
844d381
removing debugging line
matiasbenedetto Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,15 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
$used_layout = $default_layout;
}

$id = uniqid();
$style = gutenberg_get_layout_style( ".wp-container-$id", $used_layout, $has_block_gap_support );
$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'] . ' ' : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@matiasbenedetto matiasbenedetto Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

// This assumes the hook only applies to blocks with a single wrapper.
// I think this is a reasonable limitation for that particular hook.
$content = preg_replace(
'/' . preg_quote( 'class="', '/' ) . '/',
'class="wp-container-' . $id . ' ',
'class="' . $container_class . $justify_class,
$block_content,
1
);
Expand Down
61 changes: 42 additions & 19 deletions packages/block-library/src/query-pagination-next/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,40 +17,63 @@
function render_block_core_query_pagination_next( $attributes, $content, $block ) {
$page_key = isset( $block->context['queryId'] ) ? 'query-' . $block->context['queryId'] . '-page' : 'query-page';
$page = empty( $_GET[ $page_key ] ) ? 1 : (int) $_GET[ $page_key ];
$paged = ( get_query_var( 'paged' ) ) ? get_query_var( 'paged' ) : 1;
$max_page = isset( $block->context['query']['pages'] ) ? (int) $block->context['query']['pages'] : 0;

$wrapper_attributes = get_block_wrapper_attributes();
$default_label = __( 'Next Page' );
$label = isset( $attributes['label'] ) && ! empty( $attributes['label'] ) ? $attributes['label'] : $default_label;
$pagination_arrow = get_query_pagination_arrow( $block, true );
$wrapper_attributes = get_block_wrapper_attributes();
$hidden_wrapper_attributes = get_block_wrapper_attributes( array( 'aria-hidden' => 'true' ) );
$default_label = __( 'Next Page' );
$label = isset( $attributes['label'] ) && ! empty( $attributes['label'] ) ? $attributes['label'] : $default_label;
$pagination_arrow = get_query_pagination_arrow( $block, true );
$content = '';

if ( $pagination_arrow ) {
$label .= $pagination_arrow;
}
$content = '';

// Check if the pagination is for Query that inherits the global context.
if ( isset( $block->context['query']['inherit'] ) && $block->context['query']['inherit'] ) {
global $wp_query;
// Take into account if we have set a bigger `max page`
// than what the query has.
$max_page = ! $max_page || $max_page > $wp_query->max_num_pages ? $wp_query->max_num_pages : $max_page;
$filter_link_attributes = function() use ( $wrapper_attributes ) {
return $wrapper_attributes;
};
add_filter( 'next_posts_link_attributes', $filter_link_attributes );
// Take into account if we have set a bigger `max page`
// than what the query has.
global $wp_query;
if ( $max_page > $wp_query->max_num_pages ) {
$max_page = $wp_query->max_num_pages;

// If there are pages to paginate.
if ( 1 < $max_page ) {
if ( (int) $max_page !== $paged ) { // If we are NOT in the last one.
$content = get_next_posts_link( $label, $max_page );
} else { // If we are in the last one.
$content = sprintf(
'<span %1$s>%2$s</span>',
$hidden_wrapper_attributes,
$label
);
}
}
$content = get_next_posts_link( $label, $max_page );
remove_filter( 'next_posts_link_attributes', $filter_link_attributes );
} elseif ( ! $max_page || $max_page > $page ) {
$custom_query = new WP_Query( build_query_vars_from_query_block( $block, $page ) );
if ( (int) $custom_query->max_num_pages !== $page ) {
$content = sprintf(
'<a href="%1$s" %2$s>%3$s</a>',
esc_url( add_query_arg( $page_key, $page + 1 ) ),
$wrapper_attributes,
$label
);
$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;
// If there are pages to paginate.
if ( 1 < $max_num_pages ) {
if ( (int) $max_num_pages !== $page ) { // If we are NOT in the last one.
$content = sprintf(
'<a href="%1$s" %2$s>%3$s</a>',
esc_url( add_query_arg( $page_key, $page + 1 ) ),
$wrapper_attributes,
$label
);
} else { // If we are in the last one.
$content = sprintf(
'<span %1$s>%2$s</span>',
$hidden_wrapper_attributes,
$label
);
}
}
wp_reset_postdata(); // Restore original Post Data.
}
Expand Down
58 changes: 45 additions & 13 deletions packages/block-library/src/query-pagination-previous/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,63 @@
function render_block_core_query_pagination_previous( $attributes, $content, $block ) {
$page_key = isset( $block->context['queryId'] ) ? 'query-' . $block->context['queryId'] . '-page' : 'query-page';
$page = empty( $_GET[ $page_key ] ) ? 1 : (int) $_GET[ $page_key ];
$paged = ( get_query_var( 'paged' ) ) ? get_query_var( 'paged' ) : 1;
$max_page = isset( $block->context['query']['pages'] ) ? (int) $block->context['query']['pages'] : 0;

$wrapper_attributes = get_block_wrapper_attributes();
$hidden_wrapper_attributes = get_block_wrapper_attributes( array( 'aria-hidden' => 'true' ) );
$default_label = __( 'Previous Page' );
$label = isset( $attributes['label'] ) && ! empty( $attributes['label'] ) ? $attributes['label'] : $default_label;
$pagination_arrow = get_query_pagination_arrow( $block, false );
$content = '';

$wrapper_attributes = get_block_wrapper_attributes();
$default_label = __( 'Previous Page' );
$label = isset( $attributes['label'] ) && ! empty( $attributes['label'] ) ? $attributes['label'] : $default_label;
$pagination_arrow = get_query_pagination_arrow( $block, false );
if ( $pagination_arrow ) {
$label = $pagination_arrow . $label;
}
$content = '';
// Check if the pagination is for Query that inherits the global context
// and handle appropriately.
if ( isset( $block->context['query']['inherit'] ) && $block->context['query']['inherit'] ) {
global $wp_query;
$max_page = ! $max_page || $max_page > $wp_query->max_num_pages ? $wp_query->max_num_pages : $max_page;
$filter_link_attributes = function() use ( $wrapper_attributes ) {
return $wrapper_attributes;
};

add_filter( 'previous_posts_link_attributes', $filter_link_attributes );
$content = get_previous_posts_link( $label );

// If there are pages to paginate...
if ( 1 < $max_page ) {
if ( 1 !== $paged ) { // ... and we are NOT in the first one.
$content = get_previous_posts_link( $label );
} else { // ... and we are in the first one.
$content = sprintf(
'<span %1$s>%2$s</span>',
$hidden_wrapper_attributes,
$label
);
}
}
remove_filter( 'previous_posts_link_attributes', $filter_link_attributes );
} elseif ( 1 !== $page ) {
$content = sprintf(
'<a href="%1$s" %2$s>%3$s</a>',
esc_url( add_query_arg( $page_key, $page - 1 ) ),
$wrapper_attributes,
$label
);
} 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@matiasbenedetto matiasbenedetto Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// If there are pages to paginate...
if ( 1 < $max_num_pages ) {
if ( 1 !== $page ) { // ... and we are NOT in the first one.
$content = sprintf(
'<a href="%1$s" %2$s>%3$s</a>',
esc_url( add_query_arg( $page_key, $page - 1 ) ),
$wrapper_attributes,
$label
);
} else { // ... and we are in the first one.
$content = sprintf(
'<span %1$s>%2$s</span>',
$hidden_wrapper_attributes,
$label
);
}
}
}
return $content;
}
Expand Down
14 changes: 12 additions & 2 deletions packages/block-library/src/query-pagination/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,17 @@ $pagination-margin: 0.5em;
}
}

&.aligncenter {
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That behavior is achieved using the centered justification:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@matiasbenedetto matiasbenedetto Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// Non-clickeable previous and next elements are not visible
>span.wp-block-query-pagination-next,
>span.wp-block-query-pagination-previous {
visibility: hidden;
}

&.wp-justify-left,
&.wp-justify-right {
> [aria-hidden="true"] {
display: none;
}
}

}