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

feat(component): provide props for customising pagination button labels #1008

Merged
merged 4 commits into from
Oct 18, 2022

Conversation

BC-SEven
Copy link
Contributor

@BC-SEven BC-SEven commented Oct 12, 2022

What?

Adding a label, previousLabel , nextLabel and getLabel prop to the Pagination component that customise the labels of the navigation buttons and range selector

Why?

Allow greater customisability of the component.

Screenshots/Screen Recordings

Screen.Recording.2022-10-12.at.4.08.54.pm.mov

Testing/Proof

Added jest spec tests to define behaviour of new pagination props.

Copy link
Contributor

@chanceaclark chanceaclark left a comment

Choose a reason for hiding this comment

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

This is something we've been meaning to get around to but haven't yet! Thanks for taking the time and effort to do so! I have a few comments, otherwise looks really good! 🙇

Comment on lines 21 to 24
previousPageLabel?: string;
nextPageLabel?: string;
getRangeLabel?(start: number, end: number, totalItems: number): string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall I think this is great! I'd love to sort of condense the names just a bit and add one more if that cool? Generally, I'd like this to align with Atlaskit: https://atlassian.design/components/pagination/code

Suggested change
previousPageLabel?: string;
nextPageLabel?: string;
getRangeLabel?(start: number, end: number, totalItems: number): string;
label?: string;
previousLabel?: string;
nextLabel?: string;
getRangeLabel?(start: number, end: number, totalItems: number): string;

The new addition label would be for aria-label="pagination".

Comment on lines 99 to 102
if (getRangeLabel === null) {
getRangeLabel = (start: number, end: number, totalItems: number): string =>
start === end ? `${start} of ${totalItems}` : `${start} - ${end} of ${totalItems}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we extract this outside of the component (same file) and default prop the getRangeLabel? I would also love to get rid of the ternary statement too 😬

const DEFAULT_GET_RANGE_LABEL: PaginationProps['getRangeLabel']  = (start, end, totalItems) => {
  if (start === end) {
    return `${start} of ${totalItems}`;
  }

  return `${start} - ${end} of ${totalItems};
}

onPageChange,
totalItems,
}) => {
(props: TablePaginationProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert this back? We actually did that to exclude the margin props to enforce design consistency. (Maybe add a comment about that?

onPageChange,
totalItems,
}) => {
(props: TablePaginationProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@BC-SEven BC-SEven marked this pull request as ready for review October 13, 2022 05:14
@BC-SEven BC-SEven requested review from a team as code owners October 13, 2022 05:14
Copy link
Contributor

@davelinke davelinke left a comment

Choose a reason for hiding this comment

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

In terms of visuals I assume the getRangeLabel prop is to allow diverse languages to be used. I'm worried of people abusing it, but we can enforce misuse in demo reviews.

I got one question; Since pagers are usually found at the bottom of pages, does the dropdown open upwards in case there is not enough space for the dropdown to open downwards?

@chanceaclark
Copy link
Contributor

Since pagers are usually found at the bottom of pages, does the dropdown open upwards in case there is not enough space for the dropdown to open downwards?

Yes, our popper library takes care of that.

@chanceaclark chanceaclark merged commit ccba275 into bigcommerce:master Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants