-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
DataViews Sidebar: Display item count on DataViews sidebar #62028
DataViews Sidebar: Display item count on DataViews sidebar #62028
Conversation
Signed-off-by: Souptik Datta <[email protected]>
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @finitelydope, @jarekmorawski. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Souptik2001! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
export default function useDataViews( postType ) { | ||
// Fetch all the posts of the post type. | ||
const data = useEntityRecords( 'postType', postType, { | ||
per_page: -1, |
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.
I am a little skeptical about fetching all the results using -1 like this.
But I am not able to think about any better way. Please suggest if you get any better approach. 🙏
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.
If you just want the total count, did you try looking at totalItems
returned by useEntityRecords?
Example:
const { totalItems } = useEntityRecords( 'postType', 'post', { status: [ 'any', 'trash' ] } ) );
useEntityRecords
makes a call to the getEntityRecordsTotalItems
selector, which returns the count sent via response headers in X-WP-Total when calling getEntityRecords.
Or you call getEntityRecordsTotalItems
directly maybe.
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.
Trying something over here with a new route and wp_count_posts
Thanks for the PR :)
Good question, I think the count should be representative of the visible records, as it is in wp-admin. So in this case the count on 'All pages' should be 2. |
Thanks @jameskoster ! I have updated the logic to exclude trash pages count from All Pages view. |
filters: [ | ||
{ | ||
field: 'status', | ||
operator: OPERATOR_IS_NONE, | ||
value: 'trash', | ||
}, | ||
], |
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.
Although this filter is not needed for the query which is run to fetch all the pages, because trash
is excluded by default, but this needed in the JS filter we are doing here - https://github.com/WordPress/gutenberg/pull/62028/files#diff-67163f722d76f852faaff5a7c0c6046d411846fc118479e62b70273363272d5cR27
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.
Why are we doing JS filtering? IMO, we shouldn't because the REST API can be altered and extended in multiple ways so the only way to get a reliable result is to use the same REST API used in the dataviews.
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.
I need to spend more time with data views, so I can't respond to the above point, but I don't think we need this filter here at all. The resulting filter badge, when clicked, does nothing.
@@ -51,6 +52,7 @@ export default function DataViewItem( { | |||
<SidebarNavigationItem | |||
icon={ iconToUse } | |||
{ ...linkInfo } | |||
suffix={ <span>{ count }</span> } |
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.
Is the span needed? 🤔
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 span
is not needed, but if count
is 0
it might mess up with the boolean logic:
{ 'with-suffix': ! withChevron && suffix }, |
Passing an object (which the React element <span />
essentially is) will always evaluate to true
, bypassing that problem.
But again, the span
is not needed. The same can be achieved with a React.Fragment
:
suffix={ <span>{ count }</span> } | |
suffix={ <>{ count }</> } |
Alternatively, we'd alter suffix
to deal better with 0
values, but this sounds like overkill.
I left one little comment, but in terms of design this seems good. Just needs a code review :) |
This is an enhancement, so punting to WP 6.7. |
I think this just needs a code review 🙏 |
@WordPress/gutenberg-core Would appreciate a review of this one! It might need a rebase at this point, though. |
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.
Looks good to me and tests well locally. Needs a rebase, and there are some open conversations pending response @Souptik2001.
I actually tried to perform the rebase myself, but then realized the branch is in a fork and not this repo, so I can't push to it.
@DaniGuardiola Would you be up for performing this rebase, then pushing to a branch directly in based on the core repo? We can credit the author using this. |
I'm happy to rebase today and implement the suggestion to use totalItems count since I made it 😄 I'll leave it open to make sure @DaniGuardiola is still happy with it, or wants to make further tweaks. |
I have a rebased PR over at #65223 (I also couldn't push to this branch), based on the recent changes to trunk. The PR is essentially the same as this one in terms of functionality. In my testing, it does expose some inconsistencies and bugs with the filtering: All pages doesn't include trashed items, but the count does. Should we include trashed items in the default view? Toggling status filters (and sometimes author filters) crashes the browser. This is from trunk. Kapture.2024-09-11.at.13.36.40.mp4 |
PR to fix that I think: |
@jameskoster I believed you worked on some mockups here. Any instincts? Mine would be to omit the trashed items from the count. |
Agreed. I left a comment in #65223 to that effect 👍 |
Closing this in favour of #65223. Let's remember to props @Souptik2001 for the good work here. |
This PR pulls across @Souptik2001's work from #62028 and using useEntityRecords to get record counts. It also: - Adds a `navigationItemSuffix` prop to DataViewItem so that consumers can pass a suffix to the SidebarNavigationItem component. - Refactors custom count hook to include default dataviews that it doesn't affect other components Reduce RHS padding Co-authored-by: ramonjd <[email protected]> Co-authored-by: Souptik2001 <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: DaniGuardiola <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: jasmussen <[email protected]> Unlinked contributors: finitelydope, jarekmorawski.
This PR pulls across @Souptik2001's work from WordPress/gutenberg#62028 and using useEntityRecords to get record counts. It also: - Adds a `navigationItemSuffix` prop to DataViewItem so that consumers can pass a suffix to the SidebarNavigationItem component. - Refactors custom count hook to include default dataviews that it doesn't affect other components Reduce RHS padding Co-authored-by: ramonjd <[email protected]> Co-authored-by: Souptik2001 <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: DaniGuardiola <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: jasmussen <[email protected]> Unlinked contributors: finitelydope, jarekmorawski. Source: WordPress/gutenberg@338940a
What?
Display item count on DataViews sidebar.
Why?
Closes: #57072
As per the issue -
In the Pattern management sidebar each category is represented by a menu item. Inside each menu item is a count indicating how many patterns there are in the category.
Therefore, it would be good to also display that on the "Pages" management sidebar, so that anyone can get the count of each view without visiting it.
How?
Display the page count beside each view.
Acheived by fetching all the pages and then doing a filter using Javascript.
Testing Instructions
Check the screencast below or the steps mentioned -
Testing Instructions for Keyboard
Screenshots or screencast
There is one problem as you can see in the below video, that the "All Pages" count displays three, that's because it just "All the pages" but inside it, it doesn't display the trash pages, because of which it displays only two items. So, should this be the correct behaviour, or the count should also display 2, or the trash items should also be displayed under all pages.
Screen.Recording.2024-05-27.at.6.mp4