Skip to content
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

Make the showPages prop on our Pagination components responsive #3390

Merged
merged 17 commits into from
Jul 14, 2023

Conversation

mperrotti
Copy link
Contributor

The showPages prop on both Pagination components can now accept a responsive value.

Closes #2773

Screenshots

Kapture.2023-06-07.at.16.56.12.mp4

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@mperrotti mperrotti requested review from a team and langermank June 7, 2023 20:59
@changeset-bot
Copy link

changeset-bot bot commented Jun 7, 2023

🦋 Changeset detected

Latest commit: 34bc0c8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 102.15 KB (+0.21% 🔺)
dist/browser.umd.js 102.69 KB (+0.21% 🔺)

@mperrotti mperrotti requested a review from joshblack June 7, 2023 21:03
@primer primer bot temporarily deployed to github-pages June 7, 2023 21:06 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 June 7, 2023 21:07 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 June 7, 2023 21:08 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 June 7, 2023 21:12 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 June 7, 2023 21:12 Inactive
@primer primer bot temporarily deployed to github-pages June 7, 2023 21:15 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 June 7, 2023 21:15 Inactive
@mperrotti mperrotti temporarily deployed to github-pages June 7, 2023 21:47 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 June 7, 2023 21:48 Inactive
@mperrotti mperrotti temporarily deployed to github-pages June 7, 2023 22:20 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 June 7, 2023 22:21 Inactive
@joshblack
Copy link
Member

@mperrotti sorry for the delay on this! Just wanted to ask if this is something that could be built-in, e.g. the steps are always hidden at narrow and become visible at larger breakpoints, or if showPages is needed for specific use-cases 👀 Just wanted to double-check to make sure I understand the context here.

@mperrotti
Copy link
Contributor Author

@joshblack - instead of building it in, I wanted to give users the option to change the value for different viewport sizes. For example, I might be using pagination in a narrow sidebar on wide screens, so I'd want to set showPages={{wide: false}}

@mperrotti mperrotti temporarily deployed to github-pages June 15, 2023 16:13 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 July 7, 2023 17:05 Inactive
@mperrotti mperrotti temporarily deployed to github-pages July 7, 2023 17:06 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 July 7, 2023 17:07 Inactive
@mperrotti mperrotti temporarily deployed to github-pages July 7, 2023 17:16 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 July 7, 2023 17:16 Inactive
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the stuff from container queries figured moving forward with option 2 works like you suggested would work! Just had one question on the implementation, curious what you think 👀

@@ -202,7 +238,7 @@ export function Pagination({
</Button>
</Step>
{pageCount > 0 ? (
<Step>
<Step hiddenViewportRanges={getViewportRangesToHidePages()}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the hiddenViewportRanges prop something that can be placed on TablePaginationSteps? Would be great to calculate this once and place it on the outermost node and then target it e.g.

  @media ${viewportRanges.narrow} {
    .TablePaginationSteps[data-hidden-viewport-ranges*='narrow'] > *:not(:first-child):not(:last-child) {
      display: none;
    }

    .TablePaginationSteps[data-hidden-viewport-ranges*='narrow'] > *:first-child {
      margin-inline-end: 0;
    }
  }

If not, I think this is something we should calculate once instead of as a function and just pass its value to each Step instead of messing around with useCallback() and calling it for each step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh good catch! Ok, I'll refactor 👍

@mperrotti mperrotti requested a review from joshblack July 12, 2023 20:27
@mperrotti mperrotti temporarily deployed to github-pages July 13, 2023 12:54 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3390 July 13, 2023 12:54 Inactive
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination enhancement: support responsive values for the showPages prop
3 participants