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

Bug in Pagination component if count < 6 #461

Closed
pgswow opened this issue Aug 15, 2024 · 9 comments · Fixed by #464
Closed

Bug in Pagination component if count < 6 #461

pgswow opened this issue Aug 15, 2024 · 9 comments · Fixed by #464

Comments

@pgswow
Copy link

pgswow commented Aug 15, 2024

Hello! i use pagination component with fixedItems props. if count={} of total pages <= 6 i see this (on screen shot). can this be fixed?

Reproduce: https://stackblitz.com/edit/solidjs-templates-h6gz6h?file=src%2FApp.jsx

2024-08-15_13-27-32

@dannextlogic
Copy link

I can confirm the issue

@dannextlogic
Copy link

@pgswow you didn't provide page={1} and defaultPage={1}, you can also remove the fixedItems items parameter.

Here's a fixed demo

@madaxen86
Copy link
Contributor

madaxen86 commented Aug 16, 2024

@pgswow you didn't provide page={1} and defaultPage={1}, you can also remove the fixedItems items parameter.

Here's a fixed demo

Well, removing fixedItems is not a fix.
This is clearly a bug.
Imagine you fetch items from an api. You won't know how many items/pages will be there.
If you choose to have fixed items, the pagination component should not return invalid indexes.
This edge case logic should be handled by the component.

PS: filed a PR that fixes it.

This was referenced Aug 16, 2024
madaxen86 added a commit to madaxen86/kobalte that referenced this issue Aug 18, 2024
…ngs count for low total count

as no ellipses are necessary if the total count is lower than 2x siblingCount + boundaries and
potential when items are fixed, we can just render only the count directly.

fix kobaltedev#461
@dannextlogic
Copy link

Please investigate another issue: when you delete last item on a page>=2, the pagination component will crash, showing some error RangeError: Invalid array length with these lines:

// 
	return (
		<>
			<Show when={showFirst()}>{context.renderItem(1)}</Show>

			<Show when={showFirstEllipsis()}>{context.renderEllipsis()}</Show>

			<For each={[...Array(previousSiblingCount()).keys()].reverse()}>
				{(offset) => <>{context.renderItem(context.page() - (offset + 1))}</>}
			</For>

			{context.renderItem(context.page())}

                         {'RIGHT HERE'}
			<For each={[...Array(nextSiblingCount()).keys()]}>
				{(offset) => <>{context.renderItem(context.page() + (offset + 1))}</>}
			</For>
                         {'RIGHT HERE'}

			<Show when={showLastEllipsis()}>{context.renderEllipsis()}</Show>

			<Show when={showLast()}>{context.renderItem(context.count())}</Show>
		</>
	);

@madaxen86
Copy link
Contributor

madaxen86 commented Aug 20, 2024

Please investigate another issue: when you delete last item on a page>=2, the pagination component will crash, showing some error RangeError: Invalid array length with these lines:

// 
	return (
		<>
			<Show when={showFirst()}>{context.renderItem(1)}</Show>

			<Show when={showFirstEllipsis()}>{context.renderEllipsis()}</Show>

			<For each={[...Array(previousSiblingCount()).keys()].reverse()}>
				{(offset) => <>{context.renderItem(context.page() - (offset + 1))}</>}
			</For>

			{context.renderItem(context.page())}

                         {'RIGHT HERE'}
			<For each={[...Array(nextSiblingCount()).keys()]}>
				{(offset) => <>{context.renderItem(context.page() + (offset + 1))}</>}
			</For>
                         {'RIGHT HERE'}

			<Show when={showLastEllipsis()}>{context.renderEllipsis()}</Show>

			<Show when={showLast()}>{context.renderItem(context.count())}</Show>
		</>
	);

Well that's probably because you pass a page to the Pagination component which does not exist anymore after removing the item.
An example:
You have 11 items in an array.
You render 10 items per page.
currentPage=2

    count={ Math.ceil( items().length / 10 )}
    page={currentPage}
...

Now if you delete the last item you'll only have 1x page left but page still points to page 2 - thus the error.
--> you need to recalculate the page when deleting items, e.g. simple fix:

   count={ Math.ceil( items().length / 10 )}
    page={Math.min(currentPage, Math.ceil( items().length / 10 ))}
...

Now page can't be bigger than count.
What would be even better though would be adding a recalculation to the delete action like

const delete = (index:number) => {
   ...
   setCurrentPage(prev => Math.min(prev,  items().length / 10))
}

I've added a fix for the internal page as well. So the PR will cover this as well.
PS: This was also an issue with more pages.

@dannextlogic
Copy link

@madaxen86 I don't need to do anything, I already recoded the component set with ARK-UI and it works as I expected. I would suggest that you turn all incoming props to signals and the problem will solve itself.
I had similar issues with other components of mine, signals fixes most of them.

@pgswow
Copy link
Author

pgswow commented Aug 22, 2024

@madaxen86 I don't need to do anything, I already recoded the component set with ARK-UI and it works as I expected. I would suggest that you turn all incoming props to signals and the problem will solve itself. I had similar issues with other components of mine, signals fixes most of them.

Hello! in Ark-UI is your Select component working? it doesn't pass the hidden field to the form

@dannextlogic
Copy link

Hello! in Ark-UI is your Select component working? it doesn't pass the hidden field to the form

I didn't test that yet.

@pgswow
Copy link
Author

pgswow commented Aug 22, 2024

Hello! in Ark-UI is your Select component working? it doesn't pass the hidden field to the form

I didn't test that yet.

Check it out, you might be surprised that it doesn’t work the way it should logically work =) the Ark-ui community neither on discord nor on github comments anything about this =(

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 a pull request may close this issue.

3 participants