Skip to content

Conversation

@danielmarcano
Copy link
Contributor

@danielmarcano danielmarcano commented Oct 6, 2023

Description

  • Create the Common/Pagination component, based on these Figma designs
  • It takes the following properties:
type Page = {
  url: string;
};

export type PaginationProps = {
  /**
   * One-based number of the current page
   */
  currentPage: number;
  pages: Page[];
  /**
   * The number of page buttons on each side of the current page button
   * @default 1
   */
  currentPageSiblingsCount?: number;
};
  • It displays pages following a set of rules, all documented within the unit tests:

  • When there's 6 pages or less, all the page buttons are visible

  • When there's more than the minimum amount of visible pages, and:

    • There's at least two elements between the current page's left sibling and the first page, a left ellipsis is shown
    • There's at least two elements between the current page's right sibling and the last page, a right ellipsis is shown
    • When both of the previous points are true, two ellipses are shown on either side
    • Ellipses, when shown, appear between the first or last element, and the siblings of the current element
  • It disables the previous button when the current page is the first one

  • It disables the last button when the current page is the last one

  • Proper accessibility ARIA attributes have been used, in order to make the component as accessible as possible

Validation

  • Here's a short video showcasing the stories for this new component
pagination-component.mov
  • Unit tests have been added to prevent regression errors on the entire rendering logic and behavior of the component

Related Issues

Closes #5915

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing.
  • I've covered new added functionality with unit tests if necessary.

@vercel
Copy link

vercel bot commented Oct 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2023 2:12pm

@danielmarcano danielmarcano marked this pull request as ready for review October 7, 2023 19:13
@danielmarcano danielmarcano requested a review from a team as a code owner October 7, 2023 19:13
@ovflowd
Copy link
Member

ovflowd commented Oct 8, 2023

Hi, @ovflowd, thank you for your comments and suggestions.

I have just pushed a refactor.

I have made the following changes:

  • Moved markup and styles of page list items into their own PaginationListItem sub-component
  • Did the same for Ellipsis
  • Removed magic numbers and enhanced the logic of the useGetPageElements custom hook
  • Documented the entire logic with comments, and improved the variable names
  • Updated the grid-area names to single words: pages, prev and next
  • Removed span and added tagName prop to FormattedMessage so that it rendered the span on its own
  • Inlined all objects that Prettier formatting allowed me to
  • Removed 'use client';

Please let me know if we would need anything else.

We super appreciate all the effort put in here. Thank you!

@ovflowd ovflowd added the hacktoberfest This Issue is meant for Hacktoberfest 2023 label Oct 8, 2023
@danielmarcano
Copy link
Contributor Author

Hi, @ovflowd. I have pushed all the latest changes and refactors you mentioned, including the removal of the !important rules, some more inlined objects, unit tests for PaginationListItem, and the global nextJsRouter.mjs mock so that we don't have to do so manually in each test file.

Let me know if the PR would be good to go, when you have some time.

Have a great rest of the day, and thanks for your kind feedback 🥳

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

Fantastic work! Thank you so much for your due diligence here. We really appreciate the effort put in here 🙇

I've left some final comments, but besides that, I think we're good to go.

@danielmarcano
Copy link
Contributor Author

danielmarcano commented Oct 9, 2023

Thank you, @ovflowd! I have modified nextJsRouter.mjs with the small change you mentioned. I have also left you an explanation on why I think setupFilesAfterEnv already does the job. Let me know if we would be good to go.

Also, I have a couple of final doubts, as this is my first contribution to this project:

  • Are PRs merged by collaborators who have reviewed it, or by the author of the PR once it's been approved and all comments have been resolved?

  • The CONTRIBUTING.md file mentions in the "When merging" section that "[squash]" is to be used in PRs made up of multiple commits. Is "squash and merge" already configured for each PR we create? Or should we manually squash the commits ourselves before merging the PR?

  • There's 4 required checks in this PR that are all showing the same "Expected — Waiting for status to be reported" message. These are: "Build on ubuntu-latest," "Build on windows-latest," "Lint," and "Tests." When are those checks triggered? Are they automatically triggered or is there anything else from my side that I should do to trigger them?

Thank you!

@ovflowd
Copy link
Member

ovflowd commented Oct 9, 2023

Are PRs merged by collaborators who have reviewed it, or by the author of the PR once it's been approved and all comments have been resolved?

By collaborators.

The CONTRIBUTING.md file mentions in the "When merging" section that "[squash]" is to be used in PRs made up of multiple commits. Is "squash and merge" already configured for each PR we create? Or should we manually squash the commits ourselves before merging the PR?

Automatically done by GitHub

There's 4 required checks in this PR that are all showing the same "Expected — Waiting for status to be reported" message. These are: "Build on ubuntu-latest," "Build on windows-latest," "Lint," and "Tests." When are those checks triggered? Are they automatically triggered or is there anything else from my side that I should do to trigger them?

The checks are not triggered automatically, but rather by collaborators, to avoid unsafe Actions from running.

@danielmarcano
Copy link
Contributor Author

Awesome, then, @ovflowd! If you think we are good to go, I leave the rest in your hands. Thanks for everything 🥳

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Oct 9, 2023
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Oct 9, 2023
@ovflowd ovflowd added this pull request to the merge queue Oct 9, 2023
Merged via the queue into nodejs:main with commit 7d6b69a Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest This Issue is meant for Hacktoberfest 2023

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create Pagination Component

3 participants