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

Add a display rows per page dropdown or textbox with pagination #100

Closed
Muhammad-Arsalan31 opened this issue Oct 20, 2022 · 10 comments
Closed
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Muhammad-Arsalan31
Copy link

Is your feature request related to a problem? Please describe.
It would be nice to add a dropdown to select how many rows user wants to show per page like this
image

Describe the solution you'd like
I think the prop recordPerPage should show the dropdown so that both dropdown and page info can be sync

Describe alternatives you've considered
An alternative can be to add a withLimitDropdown prop to table which if it is true it should show the limit dropdown and get the value from recordsPerPage state

<DataTable
  recordsPerPage="10"
  withLimitDropdown
/>
@icflorescu icflorescu added the enhancement New feature or request label Oct 20, 2022
@icflorescu
Copy link
Owner

This could be useful.
I don't have time to work on this at the moment, so I'll just leave my thoughts here in case somebody else is willing to pick it up and come up with a PR before I do.

If we decide to implement it, I can see it happening like this:

  • providing just the recordsPerPage property preserves the current behavior (no droprown shown);
  • providing 3 additional properties, let's call them onRecordsPerPageChange, recordsPerPageSelectOptions and recordsPerPageSelectLabel, would show the page size selector.

The new property types would be:

  • onRecordsPerPageChange: (number) => void
  • recordsPerPageSelectOptions: number[]
  • recordsPerPageSelectLabel: string, defaulting to 'Records per page'

So you'd practically end up using recordsPerPage in controlled mode.

Open to discussion: onRecordsPerPageChange, recordsPerPageSelectOptions and recordsPerPageSelectLabel are not the best names, of course. I'd rather have them called onPageSizeChange and pageSizes, but their names must be in line with the already existing recordsPerPage property (since I'm not in favor of deprecating/renaming existing props).

@icflorescu
Copy link
Owner

P.S. If someone else is willing to pick it up, please make sure to adjust the example page accordingly. It's not a feature if it's not documented and people don't know about it :-).

@icflorescu
Copy link
Owner

One more thing: we'd also make sure everything works well on small screen devices. We should probably wrap between page size selector and the pager.

@icflorescu
Copy link
Owner

Overall, it shouldn't be difficult to implement. PRs are welcomed :-).

@icflorescu icflorescu added the help wanted Extra attention is needed label Oct 20, 2022
@Muhammad-Arsalan31
Copy link
Author

Muhammad-Arsalan31 commented Oct 21, 2022

I can work on this next week (because I am not free this weekend) I just want to know if these names of props are fine.

onPageSizeChange or onPageOptionChange,
recordsPerPageSizes or recordsPerPageOptions
recordsPerPageSelectLabel or recordPerPageLabel

@icflorescu
Copy link
Owner

Let's stick with:

onRecordsPerPageChange: (number) => void
recordsPerPageOptions: number[]
recordsPerPageLabel: string, defaulting to 'Records per page'

This first one has to be in line with the already existing recordsPerPage property.

@Muhammad-Arsalan31
Copy link
Author

Muhammad-Arsalan31 commented Oct 24, 2022

Hi, I have worked on the feature and named the props as you mentioned above. but data types are different for onRecordsPerPageChange and recordsPerPageOptions they are

onRecordsPerPageChange: ((value: string | null) => void) | undefined;
recordsPerPageOptions: string[];

the reason for this is that the select component takes values as a string array

   <Select
      onChange={onPerPageOptionChange}
      value={`${recordsPerPage}`}
      size={paginationSize}
      data={recordsPerPageOptions as (string | SelectItem)[]}
      sx={{ width: '80px' }}
   />

also, I am getting a typescript error if I don't pass these props to DataTable component. I have made them optional but I don't know why this error is occurring can you help me with this

Type '{ withBorder: true; records: { id: string; firstName: string; lastName: string; email: string; birthDate: string;
 departmentId: string; }[]; columns: ({ accessor: string; width: number; } | { accessor: string; width: string; } | 
{ ...; })[]; ... 4 more ...; paginationColor: "ffaa11"; }' is not assignable to type 'IntrinsicAttributes & 
DataTableProps<{ id: string; firstName: string; lastName: string; email: string; birthDate: string; 
departmentId: string; }>'.
Translation: I was expecting a type matching A, but instead you passed B.

theses are some issues I am facing if I can fix these issues then I can submit the PR

also, the Ui is like this

image

@Muhammad-Arsalan31
Copy link
Author

Hi @icflorescu o, do you have any suggestions to fix the issues so I can submit the PR

@icflorescu
Copy link
Owner

data types are different for onRecordsPerPageChange and recordsPerPageOptions
You could convert them to numbers, because users will expect to be able to pass in numbers.

Regarding the TS error, I'm afraid I don't have enough information to understand where it comes from.

icflorescu added a commit that referenced this issue Oct 31, 2022
@icflorescu
Copy link
Owner

This should work in v1.7.13 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants