Post Navigation Link: Add border and spacing block support #64258
Conversation
akasunil
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +121 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
This will also style empty post navigation elements, e.g., 
I'm not sure what the appropriate approach would be. Adding a selector with diff --git a/packages/block-library/src/post-navigation-link/block.json b/packages/block-library/src/post-navigation-link/block.json
index 3972236599..3480bbad6a 100644
--- a/packages/block-library/src/post-navigation-link/block.json
+++ b/packages/block-library/src/post-navigation-link/block.json
@@ -71,5 +71,8 @@
}
}
},
+ "selectors": {
+ "border": ".wp-block-post-navigation-link:not(:empty)"
+ },
"style": "wp-block-post-navigation-link"
}I guess skipping serialization and only applying the style if a link exists would be the cleanest way since other styles like typography, while not noticeable, would not be applied at all on the empty element. It would be good to get @aaronrobertshaw's opinion. |
|
Maybe the underlying problem is that the wrapper element is being rendered even though no links exist? I think we could solve that by adding an early return if there are no links in the render callback function. However, we might need a style like this to ensure that the next link is right-justified even when the previous link doesn't exist. |
I thought of that but wasn't sure how important the empty element is to the structure of the block. Not rendering it at all would be the best, for sure. 👍🏻 |
|
Another option is that, We can apply styles to link tag only With this, we could avoid the miss-representation like below. Its looks like that button but only link is clickable. which would be confusing from user point of view. Hello-world-.-gutenberg.webm |
That could be an option. One thing is that the link is a sub-element of the block, and I think the styles should apply to the entire block consistently. If we target 
That's just my opinion. 😄 |
Right.
Should we handle this in separate PR or in this PR? |
Good point, I also expressed concern about this problem in this comment. However, this approach runs into problems when a background is applied, because the background is applied to the block itself:
To solve this problem, we would need to skip serialization of both color support and border support and write a lot of JS/PHP code.
I am leaning towards agreeing with this approach.
I think it's okay to include it in this PR. |
| // 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 👍
without the "empty" block wrapper, the next navigation block will align left.
This needs to be addressed. By using maring-inline-{start|end}, we should be able to handle RTL languages as well as LTR languages.
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.
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.
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.
I wonder how it'd work with global styles, presets etc. Should we strip all classnames and styles from the element?
Leveraging the Block Selectors API, we should be able to use something like the :empty pseudo selector in combination with :not() to ensure the Global Styles are only applied when they should be.
Completely untested example: .wp-block-post-navigation-link:not(:empty).
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.
…add/post-navigation-link-border-support
There was a problem hiding this comment.
Thanks for putting this PR together and the iterations on it so far 👍
I've given this a smoke test as well and it is working generally as I'd expect.
✅ Global styles apply in both editor and frontend
✅ Block instance styles override the Global Styles
✅ Box sizing allows parent-enforced width to be honored
While looking at the code changes, I had a couple of questions.
- Does it make sense to add border support without spacing support as well?
- It's hard to see a use case where a border right up against the text is desired
- Does the removal of the empty element actually solve the backwards compatibility issue?
- It does get the end result to look the same
- I wonder though if themes may already have styles targeting these navigation links that assume the presence of the empty element e.g. those targeting
:first-childetc. - Has the pattern directory been looked through for potential cases where removing the empty element may cause an issue?
- It is pretty simple to skip serialization and only apply those styles when the element isn't empty. Might that be better than shiping more CSS?
| "__experimentalDefaultControls": { | ||
| "radius": true, | ||
| "color": true, | ||
| "width": true, | ||
| "style": true | ||
| } |
There was a problem hiding this comment.
| "__experimentalDefaultControls": { | |
| "radius": true, | |
| "color": true, | |
| "width": true, | |
| "style": true | |
| } |
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.
We could include spacing support also in this PR, and will make border and spacing optional by default. I'll update the PR.
| // Return empty if the adjacent post is unavailable. | ||
| if ( empty( $content ) ) { | ||
| return ''; | ||
| } |
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.
akasunil
requested review from
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
I have added spacing support as well, Also made spacing and border support optional. Also, I have noticed something. Post Navigation Link block works as individual for next and previous link, unlike Query pagination block.
If we were to apply the styles that we did on query pagination block , We may have to keep it out of post navigation block class. and i don't think we should add css outside block class. Parent of post navigation block could be anything like column, group, grid etc. |
I might just need more caffeine but I'm not sure I follow your last comment or the issue you're trying to highlight, sorry 😅 The Post Navigation Link block is more equivalent to the Query Pagination Next / Previous blocks rather than the Query Pagination block.
Those linked styles appear to target the inner Query Pagination Previous and Query Pagination Next blocks rather than the Query Pagination block itself. I still think the styles could be applied to the appropriate Post Navigation Link block via the block's own styles. That said, I'm also still leaning towards not removing the empty element to avoid BC issues. |
Or may be i wasn't clear enough, what I meant to say was that the style we added on query pagination block make sense, as its parent block containing next and previous button as child blocks. However, post navigation link block can have any parent block like group, grid, column, row etc. So should we concern about the next link is right-justified when the previous link doesn't exist? It seems pointless to render an empty element merely for styling purposes. I know there may be some themes having styles with dependency of empty post navigation link block. But that shouldn't be a barrier to enhancing our functionality. what do you think ? |
My thoughts haven't changed. We should maintain backward compatibility whenever possible.
At this point, rendering the empty element is not only for styling purposes but also for backward compatibility. Removing an empty element in favour of additional CSS doesn't provide any change in functionality. It also seems we don't have full history and context as to why this block is the way that it is. Without that understanding that increases the risk in changing it. I don't see the empty element or additional CSS as perfect solutions. In such a case, I think the pragmatic approach is to err towards the conservative option. The potential removal of the empty element can be communicated, investigated, and discussed with the broader community separately. |
That worries me too.
Okay, Got it, I'll skip the serialization of the spacing and border styles and apply the style if a link exists. |
…add/post-navigation-link-border-support
…add/post-navigation-link-border-support
…add/post-navigation-link-border-support
|
This is how it works now. post-navigation-link-block-border-spacing-support.webm |
There was a problem hiding this comment.
Thanks for iterating here @akasunil 👍
There is actually a block supports prop that can be set in a block's theme.json called __experimentalSkipSerialization. This prevents a block support feature's classes and styles being added by default to the block's wrapper.
I think there's a benefit, in terms of maintaining the block, if we are consistent in our approach to skipping serialization. What do you think of updating this PR along the lines of other blocks that skip serialization, e.g. the Avatar block?
The linked Avatar block above is probably a straightforward example of skipping serialization, which involves:
- Skipping serialization via block.json flag,
__experimentalSkipSerialization(which is filterable by extenders) - Leverage the
useBorderPropshook in the edit component and apply the resulting classes and styles on the desired element - Collect the border style attributes and use the style engine to produce the classes and inline styles, before applying them where needed.
Does that make sense? I'm happy to elaborate further if needed 🙂
…navigation-link-border-support
…navigation-link-border-support
|
Thank you @aaronrobertshaw for your detailed feedback. I have updated PR according to your feedback. |
|
Flaky tests detected in 8dcec4d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11053725188
|
There was a problem hiding this comment.
Thanks for your patience here @akasunil!
I've given this another read through and test. There are still a couple of issues with the code.
- The block instance styles aren't always present so the PHP changes will result in warning notices being thrown.
- The idea of skipping serialization was for the styles to not be applied when the link was empty. This PR still renders block instance styles when the link is empty.
Also, the PR description and title doesn't match what's happening e.g. adopting spacing supports as well. Can you please update them?
| $wrapper_attributes = get_block_wrapper_attributes( | ||
| array( | ||
| 'class' => $classes, | ||
| 'style' => $support_styles['style'], |
There was a problem hiding this comment.
This will throw a warning when a block instance doesn't have padding or border styles.
…navigation-link-border-support
akasunil
changed the title
…navigation-link-border-support
There was a problem hiding this comment.
Thanks for your patience here @akasunil 👍
Unfortunately, there are still a couple of issues we need to fix up before this will be ready to merge.
The latest updates help address part of the issue I flagged in my last review but only for borders using custom colors or with different borders per side.
Border block support can apply color classes and these need to be handled as well. I've left some inline comments that expand on this further.
Here's a quick video of what I'm seeing:
Screen.Recording.2024-10-22.at.5.01.54.pm.mov
| <div | ||
| { ...blockProps } | ||
| style={ { | ||
| ...blockProps.style, | ||
| ...borderProps.style, | ||
| ...spacingProps.style, | ||
| } } | ||
| > |
There was a problem hiding this comment.
This only applies styles from the border and spacing props. Border support can define CSS classes to apply as well e.g. has-border-color etc.
You can see this in action in the editor if you apply a border to the Group block. An example that maintains the classes from useBorderProps can be seen in the Avatar block.
One option would be to pass borderProps.className etc in as classes for the useBlockProps call here.
The reason this doesn't present visually in the editor is that the border block support forces inline styles for the colors in the event that a theme doesn't correctly load their color stylesheets in the editor. Even still, we should maintain parity between the frontend and editor for classes wherever possible.
| $support_styles = block_core_post_navigation_link_get_border_and_spacing_attributes( $attributes ); | ||
| $classes .= ! empty( $support_styles['class'] ) ? " {$support_styles['class']}" : ''; |
There was a problem hiding this comment.
Similar to the editor side flagged above, this change doesn't take into account that the border support can apply color CSS classes e.g. has-border-color, has-accent-1-border-color etc for preset colors for "flat" borders.
The result is the issue raised in my last review is still present.
What?
Add border and spacing block support to the
Post Navigation Linkblock.Part of #43247
Part of #43243
Why?
Post Navigation Linkblock is missing border and spacing support.How?
Adds the border and spacing block support in block.json
Testing Instructions
Post Navigation Linkblock's border and spacing is configurable via Global StylesPost Navigation Linkblock and Apply the border and spacing stylesWatch attached video for more information.
Screenshots or screencast
Single-Posts-.-Template-.-gutenberg-.-Editor-.-WordPress.webm