Conversation
|
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. |
| function ItemsPerPageControl() { | ||
| const { view, perPageSizes, onChangeView } = useContext( DataViewsContext ); | ||
| const pageSizeValues = perPageSizes ?? PAGE_SIZE_VALUES; | ||
| if ( perPageSizes.length === 0 ) { |
There was a problem hiding this comment.
I think it could even be if ( perPageSizes.length < 2 ) { . If there's only one option it should already be set and the user wouldn't be able to change it. Offering a toggle button like that might be confusing.
| isShowingFilter: boolean; | ||
| setIsShowingFilter: ( value: boolean ) => void; | ||
| perPageSizes?: [ number, number, number, number ]; | ||
| perPageSizes: number[]; |
There was a problem hiding this comment.
Why did we limit the page sizes to 4 in #70604? Can't it be 3 or 5? This change moves that decision to the consumer level.
There was a problem hiding this comment.
The component might get a bit crammed with a lot of options, but 3 or 5 seems alright. The component could also adapt to a dropdown when there are lots of options.
A min/max might make more sense.
BTW, there's also some docs here to update - https://github.com/WordPress/gutenberg/tree/trunk/packages/dataviews#perpagesizes--number-number-number-number-
There was a problem hiding this comment.
My thought was to align with the existing design. I don't think a randomly generated number set would fit well with the current design. That said, I do agree with using min/max values
There was a problem hiding this comment.
I've capped it to 2-6, otherwise the control is not displayed 👍
talldan
left a comment
There was a problem hiding this comment.
Left a couple of very minor comments, but change generally looks good. Changelog entry also needed.
Tests might also be good, but could have been part of the initial PR.
| isShowingFilter: boolean; | ||
| setIsShowingFilter: ( value: boolean ) => void; | ||
| perPageSizes?: [ number, number, number, number ]; | ||
| perPageSizes: number[]; |
There was a problem hiding this comment.
The component might get a bit crammed with a lot of options, but 3 or 5 seems alright. The component could also adapt to a dropdown when there are lots of options.
A min/max might make more sense.
BTW, there's also some docs here to update - https://github.com/WordPress/gutenberg/tree/trunk/packages/dataviews#perpagesizes--number-number-number-number-
| function ItemsPerPageControl() { | ||
| const { view, perPageSizes, onChangeView } = useContext( DataViewsContext ); | ||
| const pageSizeValues = perPageSizes ?? PAGE_SIZE_VALUES; | ||
| if ( perPageSizes.length === 0 ) { |
There was a problem hiding this comment.
I think it could even be if ( perPageSizes.length < 2 ) { . If there's only one option it should already be set and the user wouldn't be able to change it. Offering a toggle button like that might be confusing.



Follow-up to #70604.
What?
CustomPerPageSizesstory and add an argument to theDefaultstoryScreen.Recording.2025-07-31.at.17.58.49.mov
Testing Instructions
Open the storybook and verify you can modify the sizes and the control respond to the changes. Check the empty case works.
npm install && npm storybook:dev