DataViews Sidebar: Display item count on DataViews sidebar #65223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
|
Size Change: +221 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ramonjd
commented
Sep 11, 2024
| @@ -175,7 +218,8 @@ export function useDefaultViews( { postType } ) { | |||
| }, | |||
| ], | |||
| }, | |||
| count: counts?.trash, | |||
| }, | |||
| ]; | |||
There was a problem hiding this comment.
I thought a follow up could filter out items that have no counts, e.g., .filter( ( { count } ) => count > 0 )
Kapture.2024-09-11.at.12.43.57.mp4
There was a problem hiding this comment.
Hiding empty bundled views seems reasonable. Custom views should probably remain visible regardless.
There was a problem hiding this comment.
Custom views are unaffected. There is a snag however: it takes a tick longer to fetch the counts so you'll see a little delay on first load.
2024-09-12.13.31.18.mp4
A few options I can think of, some better than others:
- Don't filter out
0values. That means the list will appear in full, but the counts, including0items, will appear a little later. Maybe the0item could be unclickable/disabled/greyed out. - Show the full list, then filter out
0values. That means the full list will appear, but0value items will disappear. 👎🏻 - Include a loading placeholder.
- ?
There was a problem hiding this comment.
I've done 1 for now:
2024-09-12.14.02.36.mp4
There was a problem hiding this comment.
A loading indicator would be a nice enhancement but can potentially come later.
packages/edit-site/src/components/sidebar-dataviews/default-views.js
Outdated
Show resolved
Hide resolved
| @@ -68,21 +69,59 @@ const DEFAULT_POST_BASE = { | |||
| layout: defaultLayouts[ LAYOUT_LIST ].layout, | |||
| }; | |||
|
|
|||
| function useDataViewItemCounts( { postType } ) { | |||
| const { records, totalItems } = useEntityRecords( 'postType', postType, { | |||
| per_page: -1, | |||
There was a problem hiding this comment.
I probably followed the same rabbit hole as @Souptik2001 here.
Unfortunately I couldn't find another way either.
getEntityRecordsTotalItems won't return anything unless there's been a getEntityRecords query to populate the state, so we have to fetch the items.
Ideally, I guess, there'd be an options request with a limited database query SELECT COUNT to set the X-WP-Total header or something... I'm not sure
ramonjd
added
[Type] Enhancement
ramonjd
requested review from
oandregal,
ntsekouras,
jameskoster,
jasmussen and
DaniGuardiola
ramonjd
force-pushed
the
add/dataviews-sidebar-display-item-count
branch
from
79ff387 to
21dfb4e
Compare
|
Thanks for picking this up :) Could we update the spacing to match the Patterns page?
I assume adding counts to Custom views would be a separate effort? How about views in the Templates section? |
DaniGuardiola
approved these changes
Sep 12, 2024
ramonjd
force-pushed
the
add/dataviews-sidebar-display-item-count
branch
from
21dfb4e to
eae8143
Compare
|
Thanks for testing, folks!
I'm not yet familiar enough with the views to give a 100% certain answer, but my 99% hunch is that yes, it'd require another piece of work.
👍🏻 To explain the discrepancy:
|
ramonjd
commented
Sep 12, 2024
There was a problem hiding this comment.
Grabbing the counts is a tad resource intensive.
I've been testing with a test with 100+ pages. It's not too bad, but who knows how it will fare with sites with 1000s of pages.
I haven't looked too deeply, but I'd guess there's a bit of effort involved creating a more performant count. We could probably create a custom endpoint to return $wpdb->get_var("SELECT COUNT(*)...
| @@ -55,6 +56,7 @@ export default function DataViewItem( { | |||
| <SidebarNavigationItem | |||
| icon={ iconToUse } | |||
| { ...linkInfo } | |||
| suffix={ navigationItemSuffix } | |||
There was a problem hiding this comment.
I added a new prop because:
- SidebarNavigationItem takes a suffix prop that we need. We want the incoming content to be clickable.
- The "outside" suffix prop is used for the drop down menu. So it should really sit outside the clickable button.
What might be a better refactor after this PR, is for <DataViewItem /> to take a sidebar item props object or something like that, which only applies to the child <SidebarNavigationItem /> e.g.,
<DataViewItem
navigationItemProps={
suffix: ...,
isActive: ...,
...
} />| @@ -18,7 +18,9 @@ export default function DataViewsSidebarContent() { | |||
| const { | |||
| params: { postType, activeView = 'all', isCustom = 'false' }, | |||
| } = useLocation(); | |||
| const defaultViews = useDefaultViews( { postType } ); | |||
|
|
|||
| const defaultViews = useDefaultViewsWithItemCounts( { postType } ); | |||
There was a problem hiding this comment.
I created a new hook because useDefaultViews() is used in several places and not all of them require fetching the count.
| @@ -15,6 +15,10 @@ | |||
| min-width: initial; | |||
| } | |||
|
|
|||
| .edit-site-sidebar-navigation-item.with-suffix { | |||
| padding-right: $grid-unit-10; | |||
| } | |||
There was a problem hiding this comment.
This is to make sure the rhs padding matches the pattern view's items.
| } | ||
|
|
||
| if ( ! records ) { | ||
| return defaultViews; |
There was a problem hiding this comment.
The list will load faster than the count, so let's return that while we wait.
|
In terms of visuals I reckon this is in a decent spot. The only outstanding issue I see is the inclusion of 
Ideally we exclude trashed items from the All pages count. |
Oh yeah, we could minus the number of trashed pages from the total count. I'll add it in. 🍺 |
…re page count to filter
…an pass a suffix to the SidebarNavigationItem component. Refactor custom count hook to include default dataviews that it doesn't affect other components Filter out empty items Reduce RHS padding
The counts will load once the API call is done.
…component. Moving styles. This is in preparation for adding colors and other background controls later.
ramonjd
force-pushed
the
add/dataviews-sidebar-display-item-count
branch
from
9f5b1e7 to
71517b4
Compare
ramonjd
commented
Sep 13, 2024
| }; | ||
|
|
||
| // All items excluding trashed items as per the default "all" status query. | ||
| counts.all = totalItems ? totalItems - counts.trash : 0; |
There was a problem hiding this comment.
Remove trash items from the all items count as per the default view (e.g., for pages:
)
|
Are folks okay to have this merged for now? |
|
I'll merge and keep an eye on any post-merge feedback 👍🏻 |
|
Hey folks! I understand that a level of efforts went into this PR but I would rather not have this feature than do unlimited page requests like this (for pages, posts...). It's fine for templates. As an side, this PR increased the "load pages" perf metric numbers. |
@youknowriad Thanks for the heads up. I'll revert today. I agree. Until we come up with an alternative, e.g., Noting that a get posts/entity count might be useful elsewhere in the future I suppose, e.g., "You have a billion attachments in your media library" 😄 |
|
Revert PR: #65625 |
|
@ramonjd curious if we should have an endpoint that just does counts for post types or expose total counts when fetching an entity regardless of the paged / per page setting. |
|
@mtias Good question. I let the cat out of the bag in relation to the custom endpoint above, but it would be also useful I imagine to have status counts exposed in entity requests as well. While What would be ideal, though I haven't looked too deeply into it, is one response for any given post type with counts for each status. It could be a route fragment with an optional field, e.g., Furthermore, if we make it embeddable in the regular |
Looks like I forgot about wp_count_posts 😄 I have a draft PR to create an endpoint around this: |
|
@finitelydope mind sharing your WordPress.org username so I can ensure you appear in the 6.7 credits listing? |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #57072
What?
Displays item count on DataViews sidebar.
Note
Props to @Souptik2001 for starting this work, and also to the following contributors: #62028 (comment) Please include these handles when merging.
Why?
From #62028
How?
From #62028
Testing Instructions
Screenshots or screencast