docs(Table): update pagination demo to match core #7896
Conversation
|
Preview: https://patternfly-react-pr-7896.surge.sh A11y report: https://patternfly-react-pr-7896-a11y.surge.sh |
| }; | ||
|
|
||
| const tableToolbar = ( | ||
| <Toolbar usePageInsets id="compact-toolbar"> |
There was a problem hiding this comment.
super small observation - the equivalent core demo here does not usePageInsets. Should they match or is the react version more correct in this case? @mcoker
There was a problem hiding this comment.
Nice catch! @kmcfaul did it right, the core demo needs to be updated. We may have left off the inset because the default inset magically aligns these checkboxes
@mcarrano should we remove that bulk select in the toolbar and move it to the table header? Like this example - https://www.patternfly.org/v4/components/table/html#checkboxes-and-actions-example
There was a problem hiding this comment.
Actually, looking at the react bulk select demo, that toolbar isn't using page insets, either.
packages/react-table/src/docs/demos/table-demos/StaticBottomPagination.jsx
Show resolved
Hide resolved
packages/react-table/src/docs/demos/table-demos/StaticBottomPagination.jsx
Outdated
Show resolved
Hide resolved
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Oh forgot to mention this, if you wrap these labels in a <div> or <span> or something, they won't stretch like this. It has to do with the way the table is displayed in mobile and the contents of the cells are converted to grid children - they stretch by default. If you wrap the children in some generic element, that element will stretch and its children won't.
|
Am I looking at the right demo? https://patternfly-react-pr-7896.surge.sh/components/table/react-demos/static-bottom-pagination-on-mobile/ I'm not seeing the pagination pop up in mobile view |
|
@mmenestr It should be at the bottom of the page. The static modifier means it does not pop up as a sticky bar in a mobile view. |
|
Gotcha! I have one more small nitpick which is that when you expand the pagination menu it shows up below which forces you to scroll to see the menu. Wonder if we should have it open above? Screen.Recording.2022-09-09.at.8.51.12.PM.mov |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LGTM, a couple of comments, neither are blockers.
| titles={{ | ||
| paginationTitle: `${variant} pagination` | ||
| }} | ||
| isStatic |
There was a problem hiding this comment.
This is adding .pf-m-static to the top pagination, too. It isn't doing anything since it only styles .pf-c-pagination.pf-m-bottom but could be confusing as to why it's there.
|
|
||
|
|
||
| const renderPagination = (variant, isCompact) => ( | ||
| <Pagination |
There was a problem hiding this comment.
This may be out of scope but I noticed a spacing difference between core and react, and looks like it's due to a difference in the toggle between the core and react demos. We recently made an update to the plain text options menu (what's used in pagination) that makes the entire toggle clickable versus just the toggle icon. @thatblindgeye added that in #7192 as a perPageComponent="button" prop on <Pagination>, but our demos that reference <Pagination> are not using it.
I dunno if it makes sense to add that to this demo and have a separate issue to add it to the other demos - or handle it some other way, but wanted to mention it.
8 participants
What: Closes #7356
Replaces the inline pagination demo with a full page pagination demo better matching core.
Also uses the updated data set in the table demos folder.