Block Library: Add the Comments Pagination Next block #36562
Conversation
|
Size Change: +289 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
gziolo
added
[Block] Comments
gziolo
changed the title
gziolo
changed the title
| } | ||
| /> | ||
| { displayArrow && ( | ||
| <span className={ `wp-block-query-pagination-next-arrow` }> |
There was a problem hiding this comment.
It looks like a copy and paste issue:
| <span className={ `wp-block-query-pagination-next-arrow` }> | |
| <span className="wp-block-comments-pagination-next-arrow"> |
There was a problem hiding this comment.
It is not 😅 . The function that renders the arrow has that class hardcoded, so, in order to be the same both in editor and frontend and also with the same styles of the query pagination, should include that name.
cbravobernal
force-pushed
the
add/comments-pagination-next
branch
2 times, most recently
from
4abd6ea
to
103068c
Compare
cbravobernal
force-pushed
the
add/comments-pagination-next
branch
from
8f8101b
to
3c72c8e
Compare
cbravobernal
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
|
Huh, I see some failing tests completely unrelated to this PR. Do they exist on |
| } | ||
|
|
||
| return sprintf( | ||
| '<div %1s>%2s</div>', |
There was a problem hiding this comment.
In the edit implementation we apply block wrapper attributes to the <a> tag. Should we align that?
There was a problem hiding this comment.
Yes, we should remove the extraneous div here.
| </span> | ||
| ) } | ||
| </a> | ||
| <div { ...useBlockProps() }> |
There was a problem hiding this comment.
Let's remove those changes from this PR, it was fixed already by @ntsekouras.
There was a problem hiding this comment.
Yes, we should not forget about this 😄
| @@ -0,0 +1 @@ | |||
| <!-- wp:comments-pagination-next /--> | |||
There was a problem hiding this comment.
Should we include some customized attributes in the test fixtures?
| * @return string Returns the next comments link for the query pagination. | ||
| */ | ||
| function render_block_core_comments_pagination_next( $attributes, $content, $block ) { | ||
| $comments_per_page = isset( $block->context['queryPerPage'] ) ? $block->context['queryPerPage'] : 4; |
There was a problem hiding this comment.
Where does the fallback value 4 come from?
There was a problem hiding this comment.
It was the default setting on Query Comment Loop a few days ago. As this is coming from the parent block. I think it would be good to remove it.
| @@ -37,17 +39,18 @@ export default function QueryPaginationEdit( { | |||
| const innerBlocks = getBlocks( clientId ); | |||
| /** | |||
| * Show the `paginationArrow` control only if a | |||
| * `QueryPaginationNext/Previous` block exists. | |||
| * `CommentsPaginationNext/Previous` block exists. | |||
There was a problem hiding this comment.
| * `CommentsPaginationNext/Previous` block exists. | |
| * Comments Pagination Next block exists. |
There was a problem hiding this comment.
Let's merge first the Comments Pagination Previous. And after that we can update with:
Comments Pagination Next and/or Previous
There was a problem hiding this comment.
Whatever works best. From my perspective, we can land this PR first because the other one still needs to be reviewed 😄
|
I updated the code in order to have the frontend styled. I'm not sure if we can still edit Attached a video with the explanation: What do you think? |
Seems so, is happening on other PRs as well. |
|
Why don't we simply use CSS to render the arrows? That way, the code won't depend on PHP functions that could change in future versions, am I right? Maybe there are some things ― RTL, accessibility, backwards compatibility, etc. ― that should be taken into consideration, and would invalidate this approach… Please, let me know. 😅 Anyway, for example, we could use the following styles for the Comments Pagination Next block inside > .wp-block-comments-pagination-next {
&::after {
margin-left: 1ch;
display: inline-block;
}
&.use-arrow-chevron::after {
content: "»";
}
&.use-arrow-arrow::after {
content: "→";
transform: scaleX(1) #{"/*rtl:scaleX(-1);*/"}; // This points the arrow right for LTR and left for RTL.
}
}…and then, in $arrow_attribute = $block->context['paginationArrow'];
$wrapper_attributes = get_block_wrapper_attributes(
array(
'class' => "use-arrow-$arrow_attribute"
)
); |
In this case, it seems to be an accessibility issue. The main issue here is the But we have to edit that function, and I don't know if we can do it or not 😓 |
Oh, I didn't know that. Thanks for the info, @c4rl0sbr4v0. 😊 |
There was a problem hiding this comment.
Thanks for your work here @c4rl0sbr4v0 !
I'm not sure if we can still edit get_query_pagination_arrow in order to work both with comments and query loops, or it's better to create a similar function just for the comments arrow.
I think it's okay to just change the function now since it's going to be introduced in 5.9, but we would need to backport the change to core.
Having said that, I don't find it bad to actually use the existing css class as well, though. If we are planning to have this display arrow functionality synced, using the .wp-block-query-pagination- prefix, could work for both these blocks as they are actually both query blocks (posts/comments) and they are already wrapped inside their parent block.
| "type": "string" | ||
| } | ||
| }, | ||
| "usesContext": [ "postId", "queryPerPage", "paginationArrow" ], |
There was a problem hiding this comment.
When we are at the place of testing these blocks inside a Query Loop we'll need to make sure to check the context name for queryPerPage as we had discussed before - aliased context if needed or not.
Not a blocker of course for now.
There was a problem hiding this comment.
Yes, it needs to be updated. I think we should also add the namespace to the pagination arrow context value.
| } | ||
|
|
||
| return sprintf( | ||
| '<div %1s>%2s</div>', |
There was a problem hiding this comment.
Yes, we should remove the extraneous div here.
| </span> | ||
| ) } | ||
| </a> | ||
| <div { ...useBlockProps() }> |
There was a problem hiding this comment.
Yes, we should not forget about this 😄
| if ( $pagination_arrow ) { | ||
| $label .= $pagination_arrow; | ||
| } | ||
| $next_comments_link = get_next_comments_link( $label, $max_page ); |
There was a problem hiding this comment.
I couldn't make the pagination work as expected. I'm not sure the implementation here is correct as we have a custom Comments query and get_next_comments_link internally uses the global query $page = get_query_var( 'cpage' );
I guess we should take into account get_approved_comments that is used in the parent block to fetch the comments and we might probably need to make a new custom query ourselves, similar to QueryPagination blocks.
There was a problem hiding this comment.
Just noting that there were some recent changes in query pagination blocks that I'll have to check first, but the fact that I couldn't make this work as expected, still stands.
There was a problem hiding this comment.
I'm updating the code with a lot of changes that were requested if you prefer to wait for them.
How is your discussion settings configured? Maybe we have a different configuration there.
Maybe we need the comment query loop working with pagination at first, although we have already the page numbers working, so I will keep taking a look at it to have it working as expected.
Regarding
I guess we should take into account get_approved_comments that is used in the parent block to fetch the comments and we might probably need to make a new custom query ourselves, similar to QueryPagination blocks.
We started a discussion about that right here! I think now is starting to get more relevant the requirement of having a multi-query loop for comments.
There was a problem hiding this comment.
Thanks for the review! It's helping quite a lot to move the comments pagination forward.
I couldn't make the pagination work as expected. I'm not sure the implementation here is correct as we have a custom Comments query and get_next_comments_link internally uses the global query $page = get_query_var( 'cpage' );
You are adding the block outside a post or page? If this help you, I have these Discussion settings:
If you are not and it is not working, we should also have to take a look at the Comment Pagination, as I think we are using there also the cpage query var.
I guess we should take into account get_approved_comments that is used in the parent block to fetch the comments and we might probably need to make a new custom query ourselves, similar to QueryPagination blocks.
We started a discussion about that here! Maybe this issue will help us understand the requirement of having multi-query comments like in the Query. I will add it to the conversation.
cbravobernal
force-pushed
the
add/comments-pagination-next
branch
from
0d526c8
to
596077b
Compare
cbravobernal
force-pushed
the
add/comments-pagination-next
branch
from
9d99849
to
9e4261d
Compare
| function add_next_comments_link_attributes() { | ||
| if ( ! function_exists( 'get_block_wrapper_attributes' ) ) { | ||
| return; | ||
| } | ||
| return get_block_wrapper_attributes(); | ||
| } | ||
| add_filter( 'next_comments_link_attributes', 'add_next_comments_link_attributes' ); |
There was a problem hiding this comment.
Is this a good way to add the attributes to the a tag?
cc: @gziolo @ntsekouras
There was a problem hiding this comment.
I think I don't know the answer. Why do we need to use the filter here? Maybe we need a different implementation fo get_next_comments_link instead?
As far as I can tell get_next_comments_link works only with the singular page so it reads everything from the WordPress's page context. If we want to support custom settings for order, items per page, etc... then we need a custom implementation.
There was a problem hiding this comment.
I haven't followed all the discussions about where the comment blocks are going, but with this approach get_next_comments_link and using the global query, I think this is a good way to add the attributes. That's what I did for Query pagination blocks. The thing is though that you should add the filter inside the render function and then you have to remove_filter before exiting.
There was a problem hiding this comment.
This is the only way I found to provide HTML attributes without a wrapper, directly into the <a> tag.
| * @return string Returns the styles and classes defined on the block editor for the block. | ||
| */ | ||
| function add_next_comments_link_attributes() { | ||
| if ( ! function_exists( 'get_block_wrapper_attributes' ) ) { |
There was a problem hiding this comment.
This function was added in WordPress 5.6 to there is no need to add the check:
| function add_next_comments_link_attributes() { | ||
| if ( ! function_exists( 'get_block_wrapper_attributes' ) ) { | ||
| return; | ||
| } | ||
| return get_block_wrapper_attributes(); | ||
| } | ||
| add_filter( 'next_comments_link_attributes', 'add_next_comments_link_attributes' ); |
There was a problem hiding this comment.
I think I don't know the answer. Why do we need to use the filter here? Maybe we need a different implementation fo get_next_comments_link instead?
As far as I can tell get_next_comments_link works only with the singular page so it reads everything from the WordPress's page context. If we want to support custom settings for order, items per page, etc... then we need a custom implementation.
| "type": "string" | ||
| } | ||
| }, | ||
| "usesContext": [ "postId", "queryPerPage", "paginationArrow" ], |
There was a problem hiding this comment.
Yes, it needs to be updated. I think we should also add the namespace to the pagination arrow context value.
| @@ -32,6 +32,6 @@ | |||
| } | |||
| } | |||
| }, | |||
| "editorStyle": "wp-block-comments-pagination-editor", | |||
| "style": "wp-block-comments-pagination" | |||
| "editorStyle": "wp-block-query-pagination-editor", | |||
There was a problem hiding this comment.
Does it mean it reuses the styles from the Query Pagination block?
I'm wondering how much overlap there really is between comments and posts. For comments, I think there are now links for older/newer comments instead of next/previous page. It doesn't mean we can't change it. However, it would be great to clarify the design.
There was a problem hiding this comment.
This doesn't seem right. If we need some styles we should copy them and add the specificity of the comments block.
There was a problem hiding this comment.
This reuses the styles due to this function , it is applying a class to the arrow with that name.
Should we create a new function to render the arrow for comments?
Could I add a new variable to set if it is a query or a comment instead?
There was a problem hiding this comment.
I'm still confused why we need a change here. And actually this results in not loading this block styles (change comments-pagination/styles.scss and check the frontend). I think we can reuse the get_query_pagination_arrow function which adds a css class like wp-block-query-pagination-..., just copy the related styles from QueryPagination styles with any small possible needed adjustments - the styles will be wrapped under each block's base css class, and finally remove this change:
get_block_wrapper_attributes( array( 'class' => 'wp-block-query-pagination' ) )
You made the above change for this styling issues, no? If not can you share the reason for adding it?
There was a problem hiding this comment.
Thanks for your patience and sorry for messing this issue so much 😓
The reason of get_block_wrapper_attributes( array( 'class' => 'wp-block-query-pagination' ) ) is because we are not getting the styles in editor / frontend, due to the block has the parent class wp-block-**comment**-pagination instead of wp-block-**query**-pagination on the parent.
For the arrow, the function get_query_pagination_arrow adds the child class wp-block-query-pagination-$pagination_type-arrow.
So, in order to have the correct styling, we need .wp-block-query-pagination .wp-block-query-pagination-type-arrow
The SCSS files on the pagination block, being the same as query pagination, won't be loaded if we only have a Comment Query Loop. That's the reason why we need them to be duplicated in both blocks.
Anyway, let's better make a small refactor to have everything clear, I'll keep you updated!
There was a problem hiding this comment.
No worries at all @c4rl0sbr4v0, there are a lot of subtle things here. You are doing great work here not messing anything! 😄
There was a problem hiding this comment.
The more I read about those styles, the more I think that we should build a completely independent logic for the Comments Pagination block. The styles used for Query Pagination are tied to the default class names generated for those blocks and it's only 50-ish lines of code:
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/query-pagination/style.scss
The PHP implementation for the get_query_pagination_arrow is 20 lines of code:
gutenberg/lib/compat/wordpress-5.9/blocks.php
Lines 23 to 43 in 78c7f0c
It hardcodes the class name and the context entry that is tied to the Query Pagination block.
|
The PR is ready to go! |
cbravobernal
force-pushed
the
add/comments-pagination-next
branch
from
a09b2e3
to
da1eeef
Compare
cbravobernal
force-pushed
the
add/comments-pagination-next
branch
from
8e97155
to
60a2e71
Compare
Description
We are adding the Next Comments block for FSE with this PR. Will close #35008 once is done.
How has this been tested?
Screenshots
https://loom.com/share/553a6762010747c7ba591a208c4a8a34
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).