Post Navigation Link: Add border and spacing block support #64258
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,11 @@ function render_block_core_post_navigation_link( $attributes, $content ) { | |
| $content = $get_link_function( $format, $link ); | ||
| } | ||
|
|
||
| // Return empty if the adjacent post is unavailable. | ||
| if ( empty( $content ) ) { | ||
| return ''; | ||
| } | ||
|
There was a problem hiding this comment. This looks right to me so far. Thank you for the changes! 🙇🏻 There's one backwards compatibility case that @t-hamano already mentioned: without the "empty" block wrapper, the next navigation block will align left. We might have to think about RTL languages too, so the "previous" block should align left? Not sure there, but we should test it. Not sure what the best thing to do here - probably CSS as @t-hamano suggests. Here's the difference I see. T4 in trunk
T4 with this PR
There was a problem hiding this comment. This code looks good to me too 👍
This needs to be addressed. By using A similar approach has been attempted for the Query Pagination block, and these two PRs may be of help: There was a problem hiding this comment. I'm not sure that simply removing the empty element solves all the backward compatibility issues here. Removing the element and adding CSS may prevent visual regressions for the default situation. There might be use cases out in the wild though where changing the block's markup breaks other theme styles. We can avoid all BC issues by only applying the styles when needed. I don't think it's a resource-intensive option and saves having to send additional CSS. There was a problem hiding this comment.
I agree that preserving the element would be the ideal approach. Some themes might expecting the element to be there for styling or structural purposes. I wonder how it'd work with global styles, presets etc. Should we strip all classnames and styles from the element? Then there's the issue of whether themes are relying on these classnames as hooks. 🤔 There was a problem hiding this comment.
Leveraging the Block Selectors API, we should be able to use something like the Completely untested example: I've recently used a loosely similar approach to ensuring List and List Item global styles are only applied to top-level blocks rather than nested lists. There was a problem hiding this comment. Good idea! Thanks for confirming. So maybe my first comment wasn't so wild as I thought. 🤣 There was a problem hiding this comment. That or we're both just wild. Either way, they say great minds think alike 🤔 I completely forgot you'd already mentioned that selector sorry. I'm a bit slow sometimes. Still, a combination of skipping serialization of the block supports, as necessary, with the custom global styles selector works for me. |
||
|
|
||
| return sprintf( | ||
| '<div %1$s>%2$s</div>', | ||
| $wrapper_attributes, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prevailing design feedback lately has been for nice applications of border support to make the border controls optional i.e. not displayed by default.
The may indicator of that has been whether the spacing controls were displayed by default. This block doesn't even have spacing support yet. I'd take that as a sign this is very edge case territory and make the controls optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could include spacing support also in this PR, and will make border and spacing optional by default. I'll update the PR.