-
Notifications
You must be signed in to change notification settings - Fork 145
docs(pagination): Content updates for accuracy and relevance. #4849
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
base: main
Are you sure you want to change the base?
Conversation
mcoker
left a comment
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.
Sweet action! I left some comments, and a number are about images. Let me know if you want help taking screenshots or setting up demos for a good screenshot.
| 1. **Top pagination:** Always right-align pagination in the top toolbar, above content views. | ||
| 1. **Top pagination:** Placed above the component it controls and aligned to the right. | ||
|
|
||
| 2. **Bottom pagination:** Placed below the component it controls. Right-aligned for desktop views and center-aligned for mobile. If you want bottom pagination to be sticky so that it remains in view as users scroll through long content, place it directly after the component it controls and refer to the [React sticky pagination](/components/pagination#sticky) or [HTML sticky pagination](/components/pagination/html#bottom-sticky) examples for implementation guidance. |
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.
FWIW it's center-aligned and should be sticky on mobile by default, so the pagination should always be placed directly after the thing it applies to so the sticky positioning works on mobile. You probably know this but the sticky variants make it sticky on desktop, too.
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.
How does breaking the guidance apart like this read to you?
Top pagination: Placed above the component it controls and aligned to the right.
Bottom pagination: Placed directly after the component it controls. On desktop, bottom pagination is right-aligned by default. On mobile, bottom pagination is center-aligned and sticky by default.
To make either top or bottom pagination sticky, so that it remains in view as users scroll through long content, refer to the React sticky pagination or HTML sticky pagination examples for implementation guidance.
packages/documentation-site/patternfly-docs/content/components/pagination/pagination.md
Show resolved
Hide resolved
...mentation-site/patternfly-docs/content/design-guidelines/components/pagination/pagination.md
Outdated
Show resolved
Hide resolved
packages/documentation-site/patternfly-docs/content/components/pagination/pagination.md
Show resolved
Hide resolved
| 1. **Items in view:** Allows the user to select the item count (number of listed items) per page, as seen in full pagination. | ||
| 1. **Per-page selector:** Used to choose the number of items displayed on a single page at once. | ||
|
|
||
| 2. **Compact pager:** Supplies the user with page-back and page-next controls only. | ||
| 2. **Full pagination:** Supplies data navigation controls, including previous page and next page, first page and last page, and a manual option to type in a desired page number. |
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.
These bullets and the purple numbers in the image don't really make sense, but it could just be me 😅 "Per page selector" is describing the 1 box in the image, but that's already covered in the full pagination docs above - do we need to include it again? The 2 box in the image points to the "next page" button, but it says "Full pagination" here with a general description of full pagination. I might just remove these 2 bullets - IMO they aren't needed. You could also link "full pagination" in the sentence above this image to the full pagination heading of this doc, which would point people to the full pagination docs covered in these bullets.
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.
oh yeah, looking back at this I have no clue what I was going for lol. Maybe made more sense before I basically rewrote this page & mayyybe I was interpreting it as trying to split the desktop into two major parts - the summary and the "full pagination" like this (which is obviously inaccurate anyways)

But either way, yeah I just tweaked the image and removed the 2 bullets
packages/documentation-site/patternfly-docs/content/components/pagination/pagination.md
Show resolved
Hide resolved
| The text indicator provides the user with a control that displays indeterminate page count or items. | ||
| 1. **Pagination summary:** A non-interactive count of items currently displayed. | ||
|
|
||
| 2. **Mobile pagination:** A type of compact pagination, with a previous page, next page, and a per-page selector, which allows users to choose the number of items shown at once. Mobile views use compact styling, which removes the first page and last page controls. |
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'm not sure if this needs to be called out, but this is just bottom pagination - by default when you shrink the page, it transforms into this magical, floaty box ✨
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.
rephrased it like this! wdyt?
- Bottom pagination: When bottom pagination shrinks in size for mobile, it becomes more compact, with a previous page, next page, and a per-page selector that allows users to choose the number of items shown at once. There are now first-page and last-page controls for bottom pagination on mobile.
...mentation-site/patternfly-docs/content/design-guidelines/components/pagination/pagination.md
Outdated
Show resolved
Hide resolved
|
|
||
| When using pagination in mobile views, follow these best practices: | ||
| - As shown in this example, place a pagination summary above the component it controls and a mobile pagination below the component. | ||
| - Do not use full pagination in both the top and bottom toolbars. |
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.
2 things about this:
- Bottom pagination shouldn't go in a toolbar, it should just be used on its own.
- Unless I'm misreading it, doesn't this conflict with the text above saying to "add a second full pagination..."?
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 "add a second full pagination..." is in reference to desktop I think, whereas this list is meant to be mobile-specific rules. I also think I misinterpreted your notes from the issue RE point 2.
How do these edits seem?
When using pagination in mobile views, follow these best practices:
- Use a top-pagination summary and bottom pagination.
- Only use full pagination for top navigation on mobile when you can't use bottom pagination, or when you can only use the bottom-pagination summary (instead of a full bottom pagination).
- Never use full pagination for both the top and bottom pagination on mobile.
...mentation-site/patternfly-docs/content/design-guidelines/components/pagination/pagination.md
Outdated
Show resolved
Hide resolved
mcoker
left a comment
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 like img/mobile-pagination.svg has an issue and isn't rendering.
Left one comment, otherwise LGTM!
| When used in a toolbar, pagination may leave limited room for other items like filters or bulk selectors. These additional items should automatically move into an [overflow menu](/components/toolbar/design-guidelines#overflow-menu) as needed. | ||
|
|
||
| 2. **Compact pager:** Supplies the user with page-back and page-next controls only. | ||
| For components with long, scrolling content (like a table), you can add a second full pagination component below the content to ensure that users can navigate without scrolling back to the top. |
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.
Just my take on it but since the top full pagination turns into a summary on mobile by default, I would expect bottom pagination to be used by default, unless you can't - otherwise you lose pagination on mobile.
And I think it's worth calling out bottom pagination as "bottom" instead of (or along with) "second full pagination below" since technically "bottom" is a variant of the pagination component (<Pagination variant="bottom">), just so it's clear. As a designer, it probably doesn't matter to call out the distinction since they look the same, but as a dev variant="bottom" is what should be used, which is what enables the sticky/centered mobile layout.
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.
mcoker
left a comment
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.
It all looks good to me! Left one comment. Also an aside, but did you mean to export the images as SVGs? Typically for a screenshot, you'd use a JPG if possible, or a PNG if you need sharp lines and things since JPG can create some fuzziness if the quality is too low due to compression. SVGs are usually just for icons and really simple images with simple shapes. Particularly best for things that need to scale in size without any loss of quality (like icons)
| ### Mobile | ||
|
|
||
| Mobile pagination is formatted differently in order to conserve space. On mobile, full pagination will collapse to a pagination summary with an item count. [Learn how to customize overflow menu breakpoints in our toolbar design guidelines](/components/toolbar/design-guidelines). | ||
| On mobile, pagination adapts in order to better conserve space. When switching from desktop to mobile, top pagination will collapse to a pagination summary with an item count and bottom pagination will adapt to a more compact design. [Learn how to customize overflow menu breakpoints in our toolbar design guidelines](/components/toolbar/design-guidelines). |
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.
Just an opinion, but I would probably only link the "toolbar design guidelines" text? It's more concise (easier to read) and accurately describes the resulting page. I also wonder why it's here - there is no mention of doing anything with overflow menus? There is a line in the desktop section that may be a more appropriate spot for this link?
Line 70 in bfd0dc5
| When used in a toolbar, pagination may leave limited room for other items like filters or bulk selectors. These additional items should automatically move into an [overflow menu](/components/toolbar/design-guidelines#overflow-menu) as 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.
Hm yeah, maybe it's not needed here. I know that the link/mention of overflow was originally in the desktop section before I hacked away at the doc - I may have just accidentally placed it within the mobile section too. I'll remove this and leave the link in the desktop section
Oh interesting, yes the majority of images across our website are SVGs, because they're images exported from Figma. We've tried exporting them as SVGs instead, but they were coming out a little fuzzy too. I remember this issue you made about image consistency across the site - I could experiment with PNGs again and retry that approach in a separate PR that also addresses your issue |

Closes #4795
I ended up doing a bigger revamp of this page, because it was pretty outdated and I felt like it needed a little more than just the smaller clarifications