Conversation
Okay, for sure. Reviewing now. Will update you shortly. |
There was a problem hiding this comment.
Pull request overview
Adds a post-details dialog so users can click a post card to view more information inline.
Changes:
- Add clickable post cards that open a details dialog in the public posts page
- Introduce dialog state management (selectedPost/showDialog) and close handling
- Add dialog UI import in the dashboard posts page (plus whitespace-only changes)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/routes/dashboard/posts/+page.svelte | Adds Dialog import (and whitespace) in preparation for a dialog feature |
| src/routes/(app)/posts/+page.svelte | Makes posts clickable and displays post details in a modal dialog |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Card.Root | ||
| class="p-6 transition-transform hover:scale-105 cursor-pointer" | ||
| onclick={() => openPost(post)} | ||
| > |
There was a problem hiding this comment.
A clickable card needs keyboard accessibility. Consider using a semantic interactive element (e.g., a or ) or add appropriate attributes/handlers (role="button", tabindex="0", and key handling for Enter/Space) so keyboard and assistive technology users can open the post dialog.
| {#if selectedPost} | ||
| <Dialog.Root open={showDialog} onOpenChange={closePost}> | ||
| <Dialog.Content class="max-w-2xl"> |
There was a problem hiding this comment.
The open-change handler unconditionally calls closePost(). If the Dialog component invokes onOpenChange with the next open state (common pattern), this will also run when opening and immediately close the dialog. Update the handler to accept the next state and only clear selectedPost when the dialog is closing (or use the library’s recommended bind pattern, e.g., bind:open, and react to open=false).
| <div class="mt-6 flex justify-end"> | ||
| <button | ||
| class="px-4 py-2 rounded bg-primary text-background" | ||
| onclick={closePost} | ||
| > | ||
| Close | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Prefer using the dialog library’s close primitive (e.g., Dialog.Close / CloseButton / asChild pattern, depending on your dialog wrapper) instead of a custom close handler. This typically ensures consistent behavior (focus restoration, correct event wiring) and reduces the risk of state drift between showDialog and the dialog’s internal state.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/routes/(app)/posts/+page.svelte
Outdated
|
|
||
| let selectedPost: any = null; |
There was a problem hiding this comment.
| let selectedPost: any = null; | |
| let selectedPost: any = $state(null); |
There was a problem hiding this comment.
@wasaniUB Since this is reactive, you need to use $state()
| const { posts } = data; | ||
|
|
||
| let selectedPost: any = null; | ||
| let showDialog = false; |
There was a problem hiding this comment.
| let showDialog = false; | |
| let showDialog = $state(false); |
Always use $state() for elements that you want to update dynamically later on
| let selectedPost: any = null; | ||
| let showDialog = false; | ||
|
|
||
| function openPost(post: any) { |
There was a problem hiding this comment.
@wasaniUB You need to have a type for post. Do not use "any". Consider making an interface
Added a feature where posts were clickable and would show more information when clicked. For the earlier pull request, I accidentally had the base branch as main and not staging.