Add styling options to query pagination numbers #38508
Conversation
carolinan
added
the
[Block] Query Pagination
|
Size Change: +2.91 kB (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
| const paginationNumbers = previewPaginationNumbers(); | ||
| return <div { ...useBlockProps() }>{ paginationNumbers }</div>; | ||
| return ( | ||
| <div { ...blockProps } style={ { ...blockProps.style, gap: blockGap } }> |
There was a problem hiding this comment.
That threw me off a bit... It seems that blockGap support is used only in layout abstraction and that's why this code is needed. Do you think this would be the proper handling of such cases? --cc @ramonjd @andrewserong @aaronrobertshaw @youknowriad
There was a problem hiding this comment.
@ntsekouras yes, I think if we're going to use blockGap here, it'd also make sense to opt-in to the __experimentalLayout support to handle generating the styles, and also server-rendering the flex display. E.g. something like the following:
"__experimentalLayout": {
"allowInheriting": false,
"allowSwitching": false,
"default": {
"type": "flex"
}
}If we do that for this block, though, the justification controls that get exposed in the UI might not make much sense in this context. So, there's possibly an opportunity here to optionally hide justification controls when opting in to that layout?
| @@ -19,7 +19,13 @@ const previewPaginationNumbers = () => ( | |||
| </> | |||
| ); | |||
|
|
|||
| export default function QueryPaginationNumbersEdit() { | |||
| export default function QueryPaginationNumbersEdit( attributes ) { | |||
There was a problem hiding this comment.
This should be renamed to props.
There was a problem hiding this comment.
Thanks for updating this block @carolinan! I've added a couple of questions about the color supports, and to the discussion on blockGap.
I think this opens up an interesting discussion surrounding how blockGap works — it's currently designed to work with blocks that opt-in to the __experimentalLayout support, where the rendering of display: flex, etc, is already handled. That'd be my preference for how to use it, rather than adding in ad hoc server-rendering within the block directly, so that the block can also support a gap value set via theme.json. It might be slightly more complex to get that working correctly, though, since I'm not sure we've opted in to the blockGap support yet for any blocks that don't also have innerBlocks. But, it could be a good opportunity for us to explore how the layout support can work for this use case?
One option, if exploring blockGap gets a bit more complex, could be to split out that change to a separate PR so that you can get the typography and color support in sooner, so that it doesn't hold up those immediately useful features.
| "html": false | ||
| "html": false, | ||
| "color": { | ||
| "gradients": true, |
There was a problem hiding this comment.
Adding a background color without any additional controls (padding, border, etc) felt a bit limited to me when I was playing around with this, though it did seem nice being able to style the background gradient:
I think I'd lean toward either also including padding (but possibly not have it one of the default controls), or leave background color to the group block.
| "html": false, | ||
| "color": { | ||
| "gradients": true, | ||
| "text": false |
There was a problem hiding this comment.
Is there a reason we don't want folks to also be able to adjust the text color? I could imagine someone wanting to create high contrast pagination buttons with a black background and white text, perhaps. I imagine if we opt in to text color, we'd need link color, too, though, so that the current page and non current page can have different colors.
There was a problem hiding this comment.
To answer both these questions, I used the same settings as the query pagination next and previous page inner blocks, for consistency.
We can add padding, but it would need to be added to the other two blocks as well, in my opinion.
The text and link color settings are on the parent "query pagination" block. (which does not have any text of its own...)
There was a problem hiding this comment.
Optionally, if I need to remove blockgap, place the padding on the numbers and dots, but then there won't be padding around the container, the set of numbers, where the background is applied.
There was a problem hiding this comment.
I used the same settings as the query pagination next and previous page inner blocks, for consistency.
Ah, good point, I see now. Thanks! I think it'd be good for all three to be able to adjust padding and individual color controls, but that could totally be something we explore in a different PR. For now, going with consistency with the other blocks sounds good to me.
There was a problem hiding this comment.
Is there a reason we don't want folks to also be able to adjust the text color?
Actually there is. These two blocks prev/next have no wrapper and are always a link(a element), but the way color supports work is to apply css rules for the children elements. For example if we added the link color support would try to target innner a elements from the block, which do not exist 😄
Now we have kept the color support to the parent Query Pagination block which can contain both links and text(in Pagination Numbers) and users can control this in one place.
| const paginationNumbers = previewPaginationNumbers(); | ||
| return <div { ...useBlockProps() }>{ paginationNumbers }</div>; | ||
| return ( | ||
| <div { ...blockProps } style={ { ...blockProps.style, gap: blockGap } }> |
There was a problem hiding this comment.
@ntsekouras yes, I think if we're going to use blockGap here, it'd also make sense to opt-in to the __experimentalLayout support to handle generating the styles, and also server-rendering the flex display. E.g. something like the following:
"__experimentalLayout": {
"allowInheriting": false,
"allowSwitching": false,
"default": {
"type": "flex"
}
}If we do that for this block, though, the justification controls that get exposed in the UI might not make much sense in this context. So, there's possibly an opportunity here to optionally hide justification controls when opting in to that layout?
|
|
||
| $wrapper_attributes = get_block_wrapper_attributes(); | ||
| $content = ''; | ||
| if ( $blockgap ) { | ||
| $wrapper_attributes = get_block_wrapper_attributes( array( 'style' => "gap:{$blockgap};display:flex;" ) ); |
There was a problem hiding this comment.
If we stick with the server-rendering here, should we add an esc_attr before concatenating into style attribute?
There was a problem hiding this comment.
Did I add it in the correct place?
|
To me the blockgap and block spacing control is ideal for the wanted end result which is to add a flexible spacing between the numbers.
|
| @@ -19,7 +19,13 @@ const previewPaginationNumbers = () => ( | |||
| </> | |||
| ); | |||
|
|
|||
| export default function QueryPaginationNumbersEdit() { | |||
| export default function QueryPaginationNumbersEdit( attributes ) { | |||
| const blockProps = useBlockProps(); | |||
There was a problem hiding this comment.
Not mandatory, but it's also possible to pass the gap style to useBlockProps so we don't have to destructure the styles inline.
export default function QueryPaginationNumbersEdit( { attributes } ) {
const blockProps = useBlockProps( {
style: {
gap: attributes?.style?.spacing?.blockGap,
},
} );
return <div { ...blockProps }>{ previewPaginationNumbers() }</div>;
}
I've tested things out by enabling layout support on the pagination numbers but settings the following:
This hides the layout controls, e.g.,
"__experimentalLayout": {
"allowSwitching": false,
"allowInheriting": false,
"allowEditing": false,
"default": {
"type": "flex"
}
}Maybe this addresses some of @andrewserong's concerns above? |
|
Thanks for testing @ramonjd — from the sounds of it, that gets us half way there. I wonder if it's worth us looking into adding a setting under Edit: on second thoughts, even if we can override the |
I've tried reading here and from my understanding, I think that's the root thing here. Since the numbers are not inner blocks, should we be allowed or not to use "blockGap". From the name of the property "blockGap", it's clearly about separating blocks, that said, I can see the convenience of having the same UI... for a spacing property to space items in a block that are not block gap. So maybe it's fine to use that way for some adhoc use-cases? Maybe it's what |
|
I added the Question: I did run |
| "blockGap": true | ||
| } | ||
| }, | ||
| "__experimentalLayout": { |
There was a problem hiding this comment.
I don't think we should be adding this unless the block uses inner blocks. The current behavior of the layout block support (apply to the wrapper of blocks) is not the right one and need to be moved to the inner blocks wrapper at some point which would break this since this block doesn't have inner blocks.
There was a problem hiding this comment.
And I think we agree that the numbers should not be inner blocks?
There was a problem hiding this comment.
Yes, I think so, it's not clear what that would bring.
|
Hi @carolinan should we separate the typography/color supports from the spacing on another PR? since that seems to need a bit more discussion but the rest is more straightforward and would make all 3 blocks more consistent together. |
|
I agree but I haven't had time to do it. Feel free to continue (and close this). |
Description
Adds some basic styling to the query pagination numbers.
Closes #38386
Testing Instructions
4, Select the query pagination numbers block.
Screenshots
query-pagination.mp4
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).