-
-
Notifications
You must be signed in to change notification settings - Fork 148
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: Add parseAsPageIndex
parser
#791
Conversation
@cenobitedk is attempting to deploy a commit to the 47ng Team on Vercel. A member of the Team first needs to authorize it. |
The only challenge I've found is that onPaginationChange: (p) => {
setPagination(p);
}, Here's the full example: const [pagination, setPagination] = useQueryStates(
{
pageIndex: parseAsPageIndex.withDefault(0),
pageSize: parseAsInteger.withDefault(15),
},
{
urlKeys: {
pageIndex: 'page',
pageSize: 'size',
},
},
);
const table = useReactTable({
columns,
data,
getCoreRowModel: getCoreRowModel(),
getPaginationRowModel: getPaginationRowModel(),
onPaginationChange: (p) => {
setPagination(p);
},
state: {
//...
pagination,
},
}); |
parseAsPageIndex
parserparseAsPageIndex
parser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
nitpick: What do you think about naming it parseAsIndex
? The zero-based approach might work for other things than pagination, though I agree it's likely to be the main use-case. parseAsIndex
might be less clear of what it does though (but that's what documentation is for).
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Fine with me! :) |
@franky47 The contribution doc is a little vague on what to add before a feature is ready. I have added the parser and test, and I have the documentation ready as well. But what about e2e test, should I update those as well? |
@franky47 Hi François. I've updated e2e tests for Next.js. As far as I can tell, everything should be ready to go 👍 |
Thank you! The e2e part wasn't really necessary as parsers can easily be unit-tested, I need to clean the e2e-next project and remove duplicates. The e2e envs are mostly to test the adapters and framework behaviours, I'll update the contribution docs to reflect that. I see the Prettier linter step is failing on the parsers file, would you mind taking a look? Thanks! |
Sure! I tried to address it in the last commit. However the output from the check was not clear as to what was wrong and when I run ![]() ![]() But I suspect it has to do with semicolon and other small things. |
Ok I might need to improve that part a bit more too then, so all tests in CI can be run locally. If you had Prettier format the files locally and it doesn't pass CI linting, maybe we have a config issue where user-set global Prettier config takes precedence and conflicts with defaults. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! I added a negative index check to the parser, to avoid out-of-bounds access.
const [value, setValue] = useQueryState('page', parseAsIndex) | ||
return ( | ||
<DemoContainer demoKey="page"> | ||
<input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Add a minimum of zero for the input, as the parser now checks for positive-only values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minimum of zero makes sense for page index, but as a general index, a value -1
or below could be utilised to iterate backwards on an array. Something like .withOptions({ min: 0})
would make sense. Was that what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I thought about going negative to do python-like backwards indexing (using .at
), but it would mean:
?page=0
maps to the last element (index -1)?page=-1
maps to the second to last element (index -2)
This seems even more confusing.
I'll remove the positive check and we can enforce runtime values with validation once #446 (or a variant) lands.
Note: TanStack Table doesn't seem to do a bounds check on negative indices: https://table.sadmn.com/?flags=advancedTable&page=-1
Awesome! Thanks for merging 🙏🏻 happy to contribute |
🎉 This PR is included in version 2.4.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hi! Thanks for a great package!
I have suggestion for a (probably very) common use-case where you add the current page (eg. of a table) to the url. At least if you are doing pagination client-side. Typically you have the page as 0-index yet you want the url to be human readable starting from 1. Here
parseAsPageIndex
comes in super handy, taking care of the +1 offset automatically.For example, let's say you use
@tanstack/react-table
and want to have the pagination as part of the url:I was also thinking about
parseAsIntegerWithOffset
with any offset number, but I could only think of very few situations where it would be beneficial.