[Block Library - Query Pagination Next/Previous]: Add an arrow attribute and sync next/previous block's arrow #33656
Conversation
|
Size Change: +699 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
I think this one would be better in the inspector, probably using a segmented control with "None | Arrow | Chevron". I'm also not sure about using svgs here, perhaps the html character is better — it needs to blend with the typographic choices the site might do, including typeface. I think we can switch to characters: |
|
Thanks for working on this @scruffian!
I agree with this! From various discussions in other PRs/issues related to adding an I also wanted to note that this seems to be a bit more convoluted, as while we can add this control in both |
Yeah, good point. I think we could either add this setting to the "Query Pagination" block instead of the individual previous next blocks, or we could have it on the individual block but sync it between the two. I can't make up my mind which one is more intuitive. 😅 One other small comment from testing this PR: We should make sure the arrow is clickable. |
After thinking more about it, I believe leaving this attribute in each block is a good way forward. So, since these blocks (QueryPaginationNext, QueryPaginationPrevious and Post Navigation Link) aren't blocks that are expected to exist many times in a page, I think we should start with the solution which doesn't introduce such complexity. |
280e02b to
b28dab4
Compare
|
I updated the approach here to remove the SVGs and use a SegmentedControl. If this approach looks good I can update the QueryPaginationPrevious and Next/Prev post blocks. |
| ( arrowControl ) => arrowControl.value === arrow | ||
| ); | ||
|
|
||
| // This can happen if the user switches from an LTR to RTL site language. |
There was a problem hiding this comment.
This can result in loosing the current value and also we can't have setAttributes this way as it produces effects. We should have it inside a useEffect.
Having said that, I think we should keep a single value and explore a css approach for mirroring them when isRtl or checking it when we render the value and change accordingly with a helper arrowsMapper object. Either way I think we should also add a span wrapper to the arrow for better handling.
I actually tried this approach with the current symbols, but seems it had a bit unexpected results. Not sure if there are some rtl browser implementation for some of them, but for example « seems to be mirroring by itself 🤔
For reference the css I tried is this (added the wrapper in rendering first):
.wp-block-query-pagination-next{
.pagiArrow{
// Flip for RTL.
transform: scaleX(1) #{"/*rtl:scaleX(-1);*/"}; // This points the chevron right for LTR and left for RTL.
}
}
There was a problem hiding this comment.
Yeah, « handles it's own RTL!
There was a problem hiding this comment.
I couldn't find where this is handled? Do you have any resource to share, to read more about it? I added the handling by css for now, but would be good to learn 😄
|
I've updated this as suggested. Ready for another review... |
There was a problem hiding this comment.
Thanks for iterating on this one. We need to make this right, as this approach might be used by other blocks.
I think that the first thing we need to decide on is if we want to have the arrow as part of the link. That will give us a better direction for the implementation.
|
I hope you don't mind I pushed a commit to try the arrow inside the link. We can revert if needed. |
|
Feel free to push anything you like to this branch :) |
| /> | ||
| <> | ||
| <InspectorControls> | ||
| <PanelBody title={ __( 'Arrow settings' ) }> |
There was a problem hiding this comment.
Do we need a panel if it's the only setting we currently have?
There was a problem hiding this comment.
We probably need it because there are other panels visible from supports, which render above this control. This is subjective though..
There was a problem hiding this comment.
Good point. Maybe this one should be just called "Settings".
There was a problem hiding this comment.
I've left as is for now and will update in a follow up when this lands: #34017, which adds an extra visual label for Segmented Control. IMO it's not clear enough when having Settings as Panel title and a plain segmented control with some options.
|
@ntsekouras the point about wanting to entangle the two buttons is a good one. Curious if this is a case to explore setting another block's attributes at the same time you set the current one. |
beaac25 to
3a74448
Compare
I'm exploring this and pushed a commit to demonstrate the syncing. It might need polishing and sanity check about the approach, but it is testable. I haven't looked at RTL though yet.. |
75cfd88 to
cf5b35c
Compare
| label={ __( 'Arrow' ) } | ||
| value={ value } | ||
| onChange={ onChange } | ||
| help={ __( |
There was a problem hiding this comment.
I've added a help text and the Arrow label is now visible. Any suggestions for the copy here are welcome.
| @@ -2,21 +2,84 @@ | |||
| * WordPress dependencies | |||
| */ | |||
| import { __ } from '@wordpress/i18n'; | |||
| import { useBlockProps, PlainText } from '@wordpress/block-editor'; | |||
There was a problem hiding this comment.
I think these blocks already shipped but both previous and next bocks look very similar, could have been variations no? or at last they could share the same code base (like we used to do in the past to generate embed blocks before refactoring to variations)
There was a problem hiding this comment.
This had been considered during the implementation, but due to existing conditional logic regarding inherit from Query, I thought it was best to leave as two separate blocks to make them more manageable.
Regarding sharing some code, that could be an option yes.
packages/block-library/src/query-pagination/query-pagination-provider.js
Outdated
Show resolved
Hide resolved
4d59281 to
30252f8
Compare
| { hasNextPreviousBlocks && ( | ||
| <InspectorControls> | ||
| <PanelBody title={ __( 'Settings' ) }> | ||
| <QueryPaginationArrowControls |
There was a problem hiding this comment.
If we want to show this control on each of the next/previous buttons, it can be prove to be a good use case for the work @gziolo is doing to show parent inspector controls in nested blocks.
There was a problem hiding this comment.
Yes, there's a family of overlapping features in expressing attributes in a parent (so it's consistent) while cascading to children in terms of being able to edit in either place.
There was a problem hiding this comment.
👍 ! That would be good and we would have to show this control to specific nested blocks only (QueryPaginationNext, QueryPaginationPrevious). QueryPagination can have (for now) QueryPaginationNumbers as well, that has nothing to do with the arrow.
I merged this PR, as the above can be in a follow up 😄
Description
Resolves: #33474
This PR offers users/themes the ability to add an arrow to the Next Page link. The advantage over this approach, rather than just allowing users/themes to set a label, is it means that in the default placeholder state, the "Next Page" text will be translated. It's also easier for users to set an arrow in this way rather than typing an HTML entity that they may not know how to type.
This PR syncs all
Query Pagination Next and Previousblocks to use the same arrow value.Testing instructions
Next/Previousblocks.Screenshots
Screen.Recording.2021-08-16.at.12.52.42.PM.mov
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).