Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Block: Add Query Total block for displaying total query results or ranges #67629

Merged
merged 16 commits into from
Dec 10, 2024

Conversation

sarthaknagoshe2002
Copy link
Contributor

Closes: #62423

What?

This PR adds a core/query-total block to display query-related information, such as the total number of results or the range of results currently displayed.

Why?

Currently, there is no block that displays the total number of query results or the range of displayed results, limiting users' ability to convey query information effectively. This block enhances usability and customization options in the block editor.

How?

  • Block Features
    • Attributes: Includes displayType to toggle between "Total Results" and "Range Display."
    • Server-Side Rendering: Dynamic data fetched using query context (queryId, query).
    • Customization: Supports typography, color, spacing, and alignment options.
  • Key Functionalities
    • "Total Results" mode: Displays the total number of query results.
    • "Range Display" mode: Shows the current range and total number of results.

Testing Instructions

  1. Add a Query Loop block to a post or page.
  2. Insert the Query Total block inside or alongside the Query Loop.
  3. Set the displayType attribute to "Total Results" or "Range Display" via block controls.
  4. Preview and verify the output:
  5. Ensure total results are displayed correctly in "Total Results" mode.
  6. Verify range calculations in "Range Display" mode.
  7. Test with various query contexts to ensure accurate dynamic rendering.

Screenshots or screencast

query-total.block.mov

Copy link

github-actions bot commented Dec 5, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sarthaknagoshe2002 <[email protected]>
Co-authored-by: fabiankaegy <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: codersantosh <[email protected]>
Co-authored-by: bhubbard <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@sarthaknagoshe2002
Copy link
Contributor Author

I’m relatively new to contributing, but I’ve taken a bold step with this PR. I apologize in advance if I’ve made any mistakes along the way!
I’m open to all kinds of feedback, especially since aspects like the block name, icon, and variation icon are open to discussion.
If I’ve overlooked any steps related to adding a new block, please let me know—I noticed a test case failing for a backport PR 😅.
I’m also working on the test cases and plan to include them either in this PR or, if acceptable, in a separate one.

@fabiankaegy fabiankaegy added [Type] Enhancement A suggestion for improvement. New Block Suggestion for a new block [Block] Query Loop Affects the Query Loop Block labels Dec 5, 2024
Copy link
Member

@fabiankaegy fabiankaegy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this in core is going to be super helpful! :) Added some initial notes 👍

packages/block-library/src/query-total/block.json Outdated Show resolved Hide resolved
packages/block-library/src/query-total/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/query-total/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/query-total/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/query-total/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/query-total/edit.js Outdated Show resolved Hide resolved
@sarthaknagoshe2002
Copy link
Contributor Author

@fabiankaegy I’ve implemented the requested changes.

Could you please help me with the failing test? I’m currently working on the fixture to address the failing unit tests, but I’m unsure about how to proceed with the other failing tests. Any guidance would be greatly appreciated!

@fabiankaegy
Copy link
Member

@fabiankaegy I’ve implemented the requested changes.

Could you please help me with the failing test? I’m currently working on the fixture to address the failing unit tests, but I’m unsure about how to proceed with the other failing tests. Any guidance would be greatly appreciated!

I think all the JS ones are due to the missing fixture.

The PHP ones are because the two functions you added are missing an @since tag in their doc comments https://github.com/WordPress/gutenberg/actions/runs/12182411187/job/33981359884?pr=67629

packages/block-library/src/query-total/index.php Outdated Show resolved Hide resolved
packages/block-library/src/query-total/index.php Outdated Show resolved Hide resolved
packages/block-library/src/query-total/index.php Outdated Show resolved Hide resolved
packages/block-library/src/query-total/index.php Outdated Show resolved Hide resolved
@gziolo
Copy link
Member

gziolo commented Dec 9, 2024

It looks like there is also an experimental block Comments Count that would be very similar. I don't know why it is still experimental, but it was raised by @carolinan recently on WordPress Slack: https://wordpress.slack.com/archives/C02QB2JS7/p1733463619396909.

In case we decide to add this Query related block, we should mirror the handling with the one related to Comments.

@sarthaknagoshe2002
Copy link
Contributor Author

sarthaknagoshe2002 commented Dec 9, 2024

In case we decide to add this Query related block, we should mirror the handling with the one related to Comments.

@gziolo here, by handling do you mean the appearance??
Cuz as far as I know the internal logic is quite consistent with other blocks like pagination numbers block as it works on similar lines.

Co-authored-by: Greg Ziółkowski <[email protected]>
@fabiankaegy
Copy link
Member

From my perspective this also is pretty much ready to go from the editorial experience.

I would love to get @jameskoster / @jasmussen to take a look a the block icon as I think that is not an ideal fit yet 🤔

Also as a follow up I would love to have a way to customize the label / omit the label entirely :)

@sarthaknagoshe2002
Copy link
Contributor Author

Also as a follow up I would love to have a way to customize the label / omit the label entirely :)

Yes, as previously discussed, I will be addressing this in a separate pull request.

// Render output based on the selected display type.
const renderDisplay = () => {
if ( displayType === 'total-results' ) {
return <div>{ __( '12 results found' ) }</div>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My last question is where the number 12 comes from. I'm not sure how much these display elements need to represent real data, but in some cases, it would be possible to show the actual number of results and the current settings for items per page.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gziolo We have a precedence for this in core. The query pagination also doesn't show the real pagination but has a good default value that showcases the block in action 🤔

I think it makes sense here also as 1 - 1 out of 1 doesn't really make sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense here also as 1 - 1 out of 1 doesn't really make sense

Now that you shared it. What should happen for edge cases?

  • 1 -1 out of 1 - should it simply display 1 or an empty string?
  • 1-5 out of 5 - should it display 1-5?

Thank you for the explanation about the number 12. It's now documented and we can proceed further 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that it's mostly covered already in PHP 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the proper answer would still be:

1 - 1 out of 1

The format should be:

index of first item on this page - index of last item on this page out of total number of results

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this is how it is handled on the frontend
Screenshot 2024-12-10 at 1 37 00 PM
Screenshot 2024-12-10 at 1 37 07 PM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the logic, it will show Displaying 1 of 1 which is perfectly fine.

@fabiankaegy
Copy link
Member

CleanShot 2024-12-10 at 08 52 38@2x

@sarthaknagoshe2002 Actually, regarding the settings sidebar. Right now you are adding an empty panel in the sidebar. We should change that to maybe move the dropdown of options from the block toolbar into the sidebar here.

@jasmussen
Copy link
Contributor

Tricky one. In fact I think all pagination, including this smmary which is related to pagination, need a visual refresh. One instinct for making them related, is putting them in a box. For now, I drew a perhaps nicer summary icon, it's not a bad metaphor:

Screenshot 2024-12-10 at 08 52 58

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M18 4H6c-1.1 0-2 .9-2 2v12c0 1.1.9 2 2 2h12c1.1 0 2-.9 2-2V6c0-1.1-.9-2-2-2Zm.5 14c0 .3-.2.5-.5.5H6c-.3 0-.5-.2-.5-.5V6c0-.3.2-.5.5-.5h12c.3 0 .5.2.5.5v12Zm-7-6-4.1 5h8.8v-3h-1.5v1.5h-4.2l2.9-3.5-2.9-3.5h4.2V10h1.5V7H7.4l4.1 5Z"/></svg>

If there's a better metaphor, let's go for it, but perhaps for now this can allow the feature to ship. And as noted, if we like it, let's follow up with better designs for the other pagination icons.

@sarthaknagoshe2002
Copy link
Contributor Author

@sarthaknagoshe2002 Actually, regarding the settings sidebar. Right now you are adding an empty panel in the sidebar. We should change that to maybe move the dropdown of options from the block toolbar into the sidebar here.

For this I have tried to maintain consistency with the search block.

Search block's Inspector Panel Search block's Toolbar
Screenshot 2024-12-10 at 1 40 06 PM Screenshot 2024-12-10 at 1 40 01 PM

@sarthaknagoshe2002
Copy link
Contributor Author

Tricky one. In fact I think all pagination, including this smmary which is related to pagination, need a visual refresh. One instinct for making them related, is putting them in a box. For now, I drew a perhaps nicer summary icon, it's not a bad metaphor:

Screenshot 2024-12-10 at 08 52 58

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M18 4H6c-1.1 0-2 .9-2 2v12c0 1.1.9 2 2 2h12c1.1 0 2-.9 2-2V6c0-1.1-.9-2-2-2Zm.5 14c0 .3-.2.5-.5.5H6c-.3 0-.5-.2-.5-.5V6c0-.3.2-.5.5-.5h12c.3 0 .5.2.5.5v12Zm-7-6-4.1 5h8.8v-3h-1.5v1.5h-4.2l2.9-3.5-2.9-3.5h4.2V10h1.5V7H7.4l4.1 5Z"/></svg>

If there's a better metaphor, let's go for it, but perhaps for now this can allow the feature to ship. And as noted, if we like it, let's follow up with better designs for the other pagination icons.

This looks cool!

Screenshot 2024-12-10 at 1 45 30 PM

@fabiankaegy
Copy link
Member

@sarthaknagoshe2002 Actually, regarding the settings sidebar. Right now you are adding an empty panel in the sidebar. We should change that to maybe move the dropdown of options from the block toolbar into the sidebar here.

For this I have tried to maintain consistency with the search block.

Search block's Inspector Panel Search block's Toolbar
Screenshot 2024-12-10 at 1 40 06 PM Screenshot 2024-12-10 at 1 40 01 PM

The main thing here if we want to keep the control in the toolbar (which I'm fine with) is that we then should remove everything in the Inspector 🤔 Right now the inspector has some text in it but no controls

@sarthaknagoshe2002
Copy link
Contributor Author

The main thing here if we want to keep the control in the toolbar (which I'm fine with) is that we then should remove everything in the Inspector 🤔 Right now the inspector has some text in it but no controls

Yes, this makes the toolbar attract the user's attention, no distraction from the Inspector Panel.

Screenshot 2024-12-10 at 2 01 34 PM

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As shared earlier, I'll approve the code aspects of the new block. It's looking solid from my perspective. Excellent collaboration @sarthaknagoshe2002. That was a fast and efficient feedback loop 🙇🏻

Let's finalize the design decisions before proceeding with merging.

Copy link
Member

@fabiankaegy fabiankaegy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well and the experience in the editor is as expected :)

Great job here! This is ready to go :)

@fabiankaegy fabiankaegy enabled auto-merge (squash) December 10, 2024 12:15
@fabiankaegy fabiankaegy merged commit e4124df into WordPress:trunk Dec 10, 2024
61 of 64 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.9 milestone Dec 10, 2024
@sarthaknagoshe2002
Copy link
Contributor Author

@fabiankaegy @gziolo Thank you so much for your invaluable support with this PR. Honestly, this feels like a significant milestone in my journey as a contributor. I truly appreciate you guiding me and making me feel at ease as a rookie.

Thanks again for carrying me through and creating such a welcoming and supportive environment!

@sarthaknagoshe2002 sarthaknagoshe2002 deleted the new-block/query-total branch December 10, 2024 18:42
@ramonjd ramonjd changed the title [ New Block ] Add Query Total block for displaying total query results or ranges New Block: Add Query Total block for displaying total query results or ranges Dec 17, 2024
yogeshbhutkar pushed a commit to yogeshbhutkar/gutenberg that referenced this pull request Dec 18, 2024
…s or ranges (WordPress#67629)

Co-authored-by: sarthaknagoshe2002 <[email protected]>
Co-authored-by: fabiankaegy <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: codersantosh <[email protected]>
Co-authored-by: bhubbard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Loop Affects the Query Loop Block New Block Suggestion for a new block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Block: Query Results Count Block
5 participants