[@react-ui] Add initialSelectedPage to Pagination Component #815
Conversation
KristinaSpasevska
requested review from
eileenmguo,
sstraatemans and
timoheddes
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
| @@ -65,6 +74,7 @@ export const Controlled: Story = { | |||
| currentPage={page} | |||
| label={label} | |||
| visiblePageLimit={visiblePageLimit} | |||
| initialSetPage={initialSetPage} | |||
There was a problem hiding this comment.
nit: Do you think initialPage would be more concise and still clear?
There was a problem hiding this comment.
Should have been initialSelectedPage , but initialPage is fine too
There was a problem hiding this comment.
initialSelectedPage is fine with me as well! initialSetPage just felt a bit strange, but I'm ok with either of the other options 👍
There was a problem hiding this comment.
Set to initialSelectedPage
There was a problem hiding this comment.
Looking at storybook. It doesn't have any selected page if the initialSelectedPage is not valid. Can you update it to set it to the first page if this is the case or implement another solution?
Also unrelated to your changes, but I realized I forgot to wrap the PageNum component in an li. Could you add that in this PR?
packages/libs/react-ui/src/components/Pagination/Pagination.stories.tsx
Outdated
Show resolved
Hide resolved
| const [page, setPage] = React.useState(1); | ||
| render: ({ totalPages, label, visiblePageLimit, initialSelectedPage }) => { | ||
| const validInitialSelectedPage = | ||
| initialSelectedPage && initialSelectedPage <= totalPages; |
There was a problem hiding this comment.
should also be greater than 0
| render: ({ totalPages, label, visiblePageLimit, initialSelectedPage }) => { | ||
| const validInitialSelectedPage = | ||
| initialSelectedPage && | ||
| initialSelectedPage <= totalPages && | ||
| initialSelectedPage > 0; | ||
| const [page, setPage] = React.useState( | ||
| validInitialSelectedPage ? initialSelectedPage : 1, | ||
| ); |
There was a problem hiding this comment.
Can this logic be removed from the story? I think the component should handle it internally
| max: 20, | ||
| step: 1, | ||
| }, | ||
| description: 'Which page should be selected at start?', |
There was a problem hiding this comment.
| description: 'Which page should be selected at start?', | |
| description: 'The default selected page before any interaction. If the `currentPage` prop is provided, it will override this value.', |
| @@ -27,6 +36,7 @@ const meta: Meta< | |||
| max: 20, | |||
| step: 2, | |||
| }, | |||
| description: 'Total number of pages', | |||
There was a problem hiding this comment.
| description: 'Total number of pages', | |
| description: 'Total number of pages.', |
| argTypes: { | ||
| label: { | ||
| control: { | ||
| type: 'text', | ||
| }, | ||
| description: 'We use this to give info to screen readers', |
There was a problem hiding this comment.
| description: 'We use this to give info to screen readers', | |
| description: 'Text that is passed to the Nav element as an aria-label for accessibility.', |
| @@ -35,6 +45,16 @@ const meta: Meta< | |||
| max: 7, | |||
| step: 1, | |||
| }, | |||
| description: 'Number of pages that would be visible', | |||
There was a problem hiding this comment.
| description: 'Number of pages that would be visible', | |
| description: 'Number of pages that are visible and can be directly selected.', |
| docs: { | ||
| description: { | ||
| component: | ||
| '<i>Note: maximum navigation items is currently limited (not technically enforced).</i><br><br><strong>Label </strong><br> is used to allow screen readers to notify user that there is a pagination here.', |
There was a problem hiding this comment.
What do you mean by this? <i>Note: maximum navigation items is currently limited (not technically enforced)
There was a problem hiding this comment.
I think @KristinaSpasevska took that from the navheader, it's not relevant to this PR or the pagination component 😁
There was a problem hiding this comment.
I took that from the NavHeader, but I thought it would be nice to notify the user that the limit we put in the storybook for the total number of pages is only for the usage of the component here (it's not a technical limitation)
There was a problem hiding this comment.
The label description is duplicated in the props section. I do think that this text overall is more confusing than it is helpful right now. I would suggest just writing a a plain text description for what the component does/is used for
KristinaSpasevska
deleted the
feat/react-ui/add-initialSelectedPage-to-pagination
branch
No description provided.