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

fix: pagination overflowing #614

Merged
merged 8 commits into from
Jan 7, 2024

Conversation

HubbeDev
Copy link
Contributor

@HubbeDev HubbeDev commented Jan 6, 2024

This pr attempts to fix #600. Im not 100% sure this is the right way to go.

Feel free to come with feedback if their is a better approach.

The fix includes

Hide every page item except current page item and ellipsis.
I also hide the "Previous" and "Next" labels on small screen to make it more tidy.

Before submitting the PR, please make sure you do the following

  • If your PR isn't addressing a small fix (like a typo), it references an issue where it is discussed ahead of time and assigned to you. In many cases, features are absent for a reason.
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Follows the contribution guidelines.
  • Format & lint the code with pnpm format and pnpm lint

Copy link

changeset-bot bot commented Jan 6, 2024

⚠️ No Changeset found

Latest commit: c35e887

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jan 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
shadcn-svelte ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 7, 2024 4:40pm

@HubbeDev HubbeDev changed the title Docs/pagination overflowing fix: pagination overflowing Jan 6, 2024
@huntabyte
Copy link
Owner

Hey @HubbeDev, thanks for taking this on! I don't think we should change the <Pagination.XX> components. We should just update the pagination-demo files to ensure it is responsive.

Copy link
Owner

@huntabyte huntabyte left a comment

Choose a reason for hiding this comment

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

Let's just update the demos to be responsive rather than the individual components. Those can remain as they are in the main branch!

@huntabyte
Copy link
Owner

huntabyte commented Jan 7, 2024

Actually what we could do is update the pagination next button and previous buttons to keep the existing API the same, but move it into a slot fallback, so something like this:

<PaginationPrimitive.PrevButton asChild let:builder>
	<Button
		variant="ghost"
		class={cn("gap-1 pl-2.5", className)}
		builders={[builder]}
		on:click
		{...$$restProps}
	>
		<slot>
			<ChevronLeft class="h-4 w-4" />
			<span class="hidden sm:block">Previous</span>
		</slot>
	</Button>
</PaginationPrimitive.PrevButton>

This way, in our updated pagination-demo components, we could do something like:

<Pagination.Item>
			<Pagination.PrevButton>
				<ChevronLeft class="h-4 w-4" />
				<span class="hidden sm:block">Previous</span>
			</Pagination.PrevButton>
</Pagination.Item>

And the same would go for the next button of course.

If you want to update how many pages there are for smaller screens, you could do something like this in the pagination-demo components:

<script lang="ts">
	import * as Pagination from "@/registry/default/ui/pagination";
	import { mediaQuery } from "svelte-legos";

	const isDesktop = mediaQuery("(min-width: 768px)");

	let count = 20;
	$: perPage = $isDesktop ? 3 : 8
</script>

or something like that!

@HubbeDev
Copy link
Contributor Author

HubbeDev commented Jan 7, 2024

@huntabyte I have moved the logic outside the components now with a slot instead.
I didn't know that svelte-legos look awesome. But I'm not 100% sure if this is the best way. The code works and updates the pagination but only on load. Not resize. So if the page is loaded on a desktop the pagination will still overflow when resizing the window.

@huntabyte
Copy link
Owner

Oh, this is actually a bug with bits-ui, the options aren't being updated with the new props. Let me push a patch fix to bits and this should resolve itself with a version update.

@huntabyte
Copy link
Owner

Just pushed a commit that fixes this issue! Thanks a ton @HubbeDev, you're greatly appreciated!

@huntabyte huntabyte merged commit 11a188e into huntabyte:main Jan 7, 2024
4 checks passed
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 this pull request may close these issues.

Pagination component overflowing its container on mobile devices
2 participants