Move Posts Per Page, Offset, and Pages controls from the block toolbar into Inspector Controls #58207
Conversation
ryanwelcher
added
[Type] Enhancement
|
Size Change: -168 B (-0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 9c18fc1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7645077201
|
|
Hey @ryanwelcher 👋 Isn't this already possible via the blocks toolbar? I think there might be some confusion as that setting only shows up when you don't have the inherit query option set. |
packages/block-library/src/query/edit/inspector-controls/per-page-control.js
Outdated
Show resolved
Hide resolved
It's a little embarrassing to admit but I didn't know that those controls existed. IMO, It's not the best UX to have controls in multiple places so do you think it makes sense to have this control in both places? |
|
@ryanwelcher you are not alone :) I've also seen this plop up manny manny times. I would agree that it is confusing. Even more so now that the grid / list setting is exposed on the post template. So there are 3 places to control the appearance. I think consolidation would go a long way here. I would try to reach out to the design folks :) |
|
My first instinct is to move all of those controls into a new ToolsPanel that would sit above the Filters one now and remove them altogether from the Toolbar. |
There was a problem hiding this comment.
I think we all agree that current control is hard to find. It would be nice to get some design feedback.
I also like the idea of grouping pagination arguments in the new tools panel since we don't have a lot of real estate in the sidebar for this block.
I agree that the inspector would be a nice (and expected) place for these controls. I'm cool with the + approach below, perhaps with just "Display", or maybe "Loop" as the panel title. We can probably come up with something better, but that's ok for now. There's also "Parameters" — a bit developer-oriented, but this block is very much developer-oriented. I would also set "Items per page" as a default tool, while having the others display when enabled via the +. May even be better to name it "Items" or "Posts" (like the columns block), and in the help text mention the pagination (not every instance of the block will have pagination).
|
|
Great feedback everyone! I'll update the PR and its scope accordingly |
ryanwelcher
changed the title
| <PerPageControl | ||
| perPage={ perPage } | ||
| offset={ offset } | ||
| onChange={ setQuery } | ||
| /> |
There was a problem hiding this comment.
Nit (non-blocking): There is probably no need to create wrappers for simple components like - PerPageControl, OffsetControl and PagesControl.
This is mostly my personal preference. Feel free to ignore it :)
There was a problem hiding this comment.
I went with wrappers to reduce the size of the index.js for this section. I guess its number of files vs lines of code. I'm happy either way, but if these components get more complicated, it would be nice to abstract that out of the main file.
|
I've made the updates to the PR. Just a note that when resetting the panels, the Per page value is not changed. We'd need to access the original value from the template to do that so I have left it out for now. |
|
Something else I think would be a good enhancement whilst we are at it is to not remove the per page control when the inherit setting is set. But instead disable it and show a hint that you can only set that when the inherit is not enabled. That would go a long way |
That's not a bad idea, my only hesitation is that if we do it for this control do we need to do it for all of them? Perhaps we can merge this and then start a discussion around what controls can/should available when the block is set to inherit as well as how to indicate that there are controls available. |
There was a problem hiding this comment.
Thank you, @ryanwelcher! The changes look good to me.
The failing e2e test (should not close the dropdown when typing in it) needs to be updated since it used display settings from the toolbar.
P.S. I need to call it a day, but I can help refactor the e2e test tomorrow. I think introducing a custom testing block will be a more future-proof solution.
|
Thanks @Mamaduka I'll take a look at those tests now. If i'm not able to address it today, I'm going to be AFK for the next two weeks so feel free to take this over the finish line! |
afercia
added
[Focus] Accessibility (a11y)
|
I'd be totally in favor of moving these settings from the block toolbar to the Inspector. For additional arguments, please see #62873 and #63020 However, I'm not sure burying down these settings into an 'ellipsis menu' would be ideal. To my understanding, this kind of 'ellipsis menu' is meant for less frequently used settings. It makes perfectly sense to not unnecessarily clutter the UI adding settings that are rarely used. Instead, I'd tend to think Regarding the 'Max page to show' setting specifically, see also #63028 |
|
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 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. |
|
I agree that more panels and access to the toolspanel menu make the UI tidier and more scalable. 'Order by', 'Sticky', and 'Force page reload' could later move to a separate panel too. The Offset control could probably use some help text to describe what it does. All controls should apply the |
|
"Max pages to show" (and the help text "Limit the pages you want to show...") reads a bit strange when the Query is configured to display posts. Would it be feasible to add logic so this copy reflects the chosen post type? Otherwise we probably need a generic term. The label could potentially be shorter too: "Max. $post_type's". |
|
Quick note: translatable strings should never use variable parts that may varay in gender for languages with gendered nouns. Instead of something like That said, and tangentially related, I'd think one of the problems with the Query Loop block inspector is that there's really too much text in it. Long descriptions that fo in 4, 5 or 6 lines are a little too much. As an accessibility specialist I should be glad when there is visible text that provides meaningful information. But, on the other hand, too much text is an accessibility barrier for some users. Screenshot:
I'd also argue that some of these descriptions are a little too 'technical' and could use a simpler, shorter, language. Consider for example this description:
Would all users understand what a I'd think that by drastically reducing the impact of these very long descriptions, adding more controls in the Inspector would be much easier. |
|
@afercia you may be interested in this issue: #63598. Also: #63599. Generic terms are tricky as they can feel ambiguous. Perhaps something like:
cc @richtabor for ideas. Otherwise I agree. The descriptions for the "Post type" and "Sticky post" controls are particularly egregious. |
There was a problem hiding this comment.
I've updated the e2e test to use a special plugin and not rely on any particular core block.
@afercia, @jameskoster, if there are no other blockers, I think PR is good to be merged.
|
@Mamaduka I think the offset control needs help text to explain what it does. The other controls could also use some copy updates, as suggested in #58207 (comment). |
|
@jameskoster, any suggestions for the "Offset" help text? |
|
Maybe something like:
It's a tough one to describe intuitively. I'd welcome other suggestions. A couple of other observations:
|
|
@ryanwelcher do you mind resolving the conflicts and rebasing with trunk? I'd like to get this iteration into trunk, then we can circle back and clean up help text/look at the next iteration. |
|
Thanks everyone for all of your input and help - such a fantastic example of how well this community works together to achieve something! I've updated the branch with latest trunk and addressed the merge conflicts. Once the tests pass, I'll merge it |
|
There was a failing Static Linting test that required add the |
ryanwelcher
deleted the
feature/add-post-count-controls-to-query-loop-block
branch
What?
This PR moves the Posts Per Page, Offset, and Max Pages to Show controls into the Inspector controls side bar and wraps them in the new ToolsPanel component.
Why?
Having controls in multiple locations (toolbar and sidebar) is bad UX and makes these controls easy to miss. I wrote a whole plugin around adding these in 😳 .
Testing Instructions
Screenshots or screencast