Update Pagination styling #1828
Conversation
This is a vestige from when this component was first created for use in the client's Notebook UI. The background color for non-active pages matched the client's background color.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1828 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 70 70
Lines 1273 1272 -1
Branches 478 477 -1
=========================================
- Hits 1273 1272 -1 ☔ View full report in Codecov by Sentry. |
| onClick: (e: MouseEvent) => void; | ||
| pressed?: boolean; | ||
| title?: string; | ||
| }; |
There was a problem hiding this comment.
I changed to explicitly listing all the used props here rather than using forwarding. This makes it easier to understand and constrain the variations in how this component is used. In general I am not a huge fan of using prop forwarding heavily except for components which are primarily intended to be styled wrappers around basic DOM components.
- Use Unicode ellipsis instead of ASCII "..." - Make the elided pages indicator approximately the same width as a small page number. Together with some future changes to how pagination items are generated, this will reduce the amount of variation in width of the Pagination component as the current page is advanced.
This fixes an issue where repeatedly pressing the prev/next page buttons would sometimes cause the text to be selected.
This fixes an issue where the Prev button was not vertically aligned with the page numbers if the font size was larger than the one used in the client, where this component originates.
This avoids the need to specify a fixed width for the container of the Prev/Next page buttons in order to keep other controls in a consistent position as the user navigates between pages, and the Prev/Next page buttons are shown/hidden.
robertknight
force-pushed
the
remove-pagination-background
branch
from
27ecc51 to
b802d7a
Compare
| // rather than removed so that the overall width of the component and | ||
| // positions of child controls doesn't change too much when navigating | ||
| // between pages. | ||
| invisible && 'invisible', |
There was a problem hiding this comment.
I reckon this should be equivalent to { invisible }
| invisible && 'invisible', | |
| { invisible }, |
There was a problem hiding this comment.
You are right. I find the current version slightly more obvious in its meaning. It also avoids a hazard of silently breaking things if the prop is renamed.
| // Disabled navigation buttons are rendered as invisible and disabled | ||
| // rather than removed so that the overall width of the component and | ||
| // positions of child controls doesn't change too much when navigating | ||
| // between pages. |
There was a problem hiding this comment.
I would even consider keeping them visible but disabled, but this should work for now without introducing design changes.
Update the styling of the Pagination control as follows:
Before:
After: