kobalte icon indicating copy to clipboard operation
kobalte copied to clipboard

Bug in Pagination component if count < 6

Open pgswow opened this issue 1 year ago • 1 comments

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

pgswow avatar Aug 15 '24 06:08 pgswow

I can confirm the issue

dannextlogic avatar Aug 15 '24 10:08 dannextlogic

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

Here's a fixed demo

dannextlogic avatar Aug 16 '24 12:08 dannextlogic

@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.

madaxen86 avatar Aug 16 '24 16:08 madaxen86

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>
		</>
	);

dannextlogic avatar Aug 20 '24 07:08 dannextlogic

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.

madaxen86 avatar Aug 20 '24 15:08 madaxen86

@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.

dannextlogic avatar Aug 21 '24 07:08 dannextlogic

@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

pgswow avatar Aug 22 '24 10:08 pgswow

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.

dannextlogic avatar Aug 22 '24 10:08 dannextlogic

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 =(

pgswow avatar Aug 22 '24 12:08 pgswow