feat: renames Pagination to OffsetPagination with revised API #1712
Conversation
geotrev
force-pushed
the
george/pagination-cleanup
branch
from
4090241 to
78beead
Compare
geotrev
force-pushed
the
george/pagination-cleanup
branch
from
78beead to
10b9bda
Compare
geotrev
force-pushed
the
george/pagination-cleanup
branch
from
10b9bda to
7a5e57f
Compare
geotrev
force-pushed
the
george/pagination-cleanup
branch
from
7a5e57f to
d896ca1
Compare
There was a problem hiding this comment.
File should be named offsetPagination.stories.mdx (lowercase "o") for consistency with others.
There was a problem hiding this comment.
[nit] let's nab this one before merge ☝️
There was a problem hiding this comment.
Dang. I really thought I pushed this one up. 🤦🏻
| GapComponent, | ||
| props, | ||
| 'aria-label', | ||
| 'Ellipsis indicating non-visible pages' |
There was a problem hiding this comment.
Is it worth passing a size prop to the gap component so that this default label can be customized with "Ellipsis indicating ${size} non-visible pages"? Certainly non-essential; perhaps nice to have.
There was a problem hiding this comment.
I like that idea. Looks like the calculation is all there already, too.
There was a problem hiding this comment.
Actually, the calculation is a bit more involved than I thought. Definitely something to consider in the future though.
There was a problem hiding this comment.
Ok, I'm totally fine to defer. But we'll probably need to punt to a future major, because we'd want the labels API to follow suit with a function.
| labels: { | ||
| gap: 'Ellipsis indicating non-visible pages', | ||
| page: p => `Page ${p}`, | ||
| next: 'Next page', | ||
| previous: 'Previous page' | ||
| }, |
There was a problem hiding this comment.
Not sure, due to the type mix. But can this be setup so it can be controlled via the story?
There was a problem hiding this comment.
Perhaps we could flatten this object & define some argTypes for it, then re-form the labels object in the Story render function?
| return css` | ||
| min-width: ${height}; | ||
| max-width: ${math(`${height} * 2`)}; /* [1] */ | ||
| max-width: ${math(`${height} * 2`)}; |
There was a problem hiding this comment.
Should we get rid of the expensive math function by making height a number calculation and appending 'px' where necessary?
| * @param {string} pageType The type of the page accepting the props | ||
| * @param {any} props Default page props to transform | ||
| * @param {number} pageNumber The page number | ||
| * Provides localized labels to pagination elements. |
There was a problem hiding this comment.
| * Provides localized labels to pagination elements. | |
| * Provides localized labels to pagination elements |
[nit] no period, per content guidelines
geotrev
force-pushed
the
george/pagination-cleanup
branch
from
0fcab89 to
2817a53
Compare
There was a problem hiding this comment.
[nit] let's nab this one before merge ☝️
Co-authored-by: Jonathan Zempel <jzempel@gmail.com>
Description
OffsetPagination(renamedPagination) with API and accessibility improvements.Detail
transformPagePropsprop has been removedlabelsprop added to more simply customize pagination element labelscontainer-paginationremoved (no arrow key navigation removed + hook-provided attributes restored in its place)navtag with translatablearia-label.aria-labelrecommended for ellipsis/page gap elementbuttonsaria-current="page"Page X(currentword removed).Checklist
👌 design updates will be Garden Designer approved (add the designer as a reviewer)npm start)?bedrock)