feat: Add better loading skeleton on Invoice Dashboard #206
Conversation
WalkthroughThe changes in this pull request involve the modification of the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
shared/components/dashboard-skeleton.svelte (2)
1-12: Consider enhancing prop names and configuration managementThe implementation could benefit from the following improvements:
- Rename props to be more semantic (e.g.,
columnCountandrowCount)- Move configuration to a shared constants file for reusability
- Use CSS variables for colors to maintain theme consistency
<script> import Skeleton from "./skeleton.svelte"; - export let rows = 7; - export let columns = 10; - let skeletonConfig = { - colors: { - main: "#D9D9D9", - secondary: "#E7E7E7", - }, - }; + export let rowCount = 7; + export let columnCount = 10; + let skeletonConfig = { + colors: { + main: "var(--skeleton-main-color, #D9D9D9)", + secondary: "var(--skeleton-secondary-color, #E7E7E7)", + }, + }; </script>
25-30: Enhance styling for better UX and accessibilityThe current styling could be improved with:
- CSS variables for colors
- Responsive design considerations
- High contrast mode support
<style> .skeleton-item { padding: 20px; - border-bottom: 1px solid #d1d5db; + border-bottom: 1px solid var(--border-color, #d1d5db); + transition: background-color 0.2s ease; } + @media (prefers-color-scheme: dark) { + .skeleton-item { + border-bottom-color: var(--border-color-dark, #4b5563); + } + } + @media (max-width: 768px) { + .skeleton-item { + padding: 16px; + } + } </style>shared/components/skeleton.svelte (2)
Line range hint
1-3: Add TypeScript type definitions for better type safetyThe
configprop lacks type definition which could lead to runtime errors if incorrect color values are provided.Consider adding proper TypeScript interfaces:
<script lang="ts"> + interface SkeletonConfig { + colors: { + main: string; + secondary: string; + } + } - export let config; + export let config: SkeletonConfig; export let lineCount = 5; </script>
Line range hint
49-56: Consider optimizing gradient animation performanceThe large background-size (400%) combined with continuous animation might impact performance, especially on lower-end devices.
Consider these optimizations:
background: linear-gradient( 90deg, var(--mainColor) 25%, var(--secondaryColor) 37%, var(--mainColor) 63% ); - background-size: 400% 400%; + background-size: 200% 100%;Also, consider adding:
will-change: opacity; transform: translateZ(0);packages/invoice-dashboard/src/lib/view-requests.svelte (1)
Line range hint
873-879: Improve table responsiveness on mobile devicesThe current implementation uses overflow scroll without any visual indication of more content, which can lead to poor user experience on mobile devices.
Consider adding visual indicators for horizontal scroll:
@media only screen and (max-width: 880px) { .table-wrapper { overflow: scroll; max-width: 900px; + position: relative; + } + + .table-wrapper::after { + content: ''; + position: absolute; + top: 0; + right: 0; + bottom: 0; + width: 5px; + background: linear-gradient(to left, rgba(0,0,0,0.05), transparent); + pointer-events: none; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/invoice-dashboard/src/lib/view-requests.svelte(3 hunks)shared/components/dashboard-skeleton.svelte(1 hunks)shared/components/skeleton.svelte(1 hunks)
🔇 Additional comments (4)
shared/components/dashboard-skeleton.svelte (1)
1-30: Verify implementation against Figma design
Please ensure that the skeleton structure matches the design reference from Figma mentioned in issue #158. The current implementation creates a generic grid pattern, but we should verify if it accurately represents the Invoice Dashboard's layout.
✅ Verification successful
The skeleton implementation matches the Invoice Dashboard structure
The dashboard-skeleton component correctly reflects the Invoice Dashboard's table structure with:
- A grid pattern matching the actual table layout (7 rows for table cells, matching columns like issue date, due date, invoice number, etc.)
- Proper spacing with padding and border styling that mirrors the main dashboard
- Skeleton placeholders that represent the loading state of actual invoice data cells
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for Invoice Dashboard component to compare structure
rg -l "Invoice.*Dashboard" --type svelte
Length of output: 73
Script:
#!/bin/bash
# Search for files related to invoice dashboard with various extensions
rg -l -g '*.{svelte,js,jsx,ts,tsx}' -i "invoice.*dashboard"
# Also search for any dashboard-related components
fd -e svelte "dashboard"
Length of output: 364
Script:
#!/bin/bash
# Let's examine the invoice dashboard implementation to understand its structure
cat packages/invoice-dashboard/src/lib/view-requests.svelte
cat packages/invoice-dashboard/src/lib/react/InvoiceDashboard.jsx
# Also check if there are any related test files that might contain layout specifications
fd -e test.{js,jsx,ts,tsx} -e spec.{js,jsx,ts,tsx} invoice
Length of output: 29175
shared/components/skeleton.svelte (2)
45-46: LGTM! Visual adjustments align with dashboard requirements
The reduced height and increased border radius better represent table row dimensions, improving the loading state visualization as per PR objectives.
Line range hint 6-12: Verify color configuration in dashboard context
Let's ensure the skeleton colors match the dashboard theme consistently.
✅ Verification successful
Color configuration is properly integrated with dashboard context
The skeleton component's color configuration is correctly integrated and consistently used across the dashboard context:
- The skeleton component receives colors through a
configprop withmainandsecondarycolors - DashboardSkeleton properly provides these colors with default values (
#D9D9D9for main,#E7E7E7for secondary) - The colors are consistently used in the invoice dashboard views, maintaining the same configuration pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for color configurations in dashboard components
rg -l "colors\.(main|secondary)" --type svelte
# Check DashboardSkeleton implementation
ast-grep --pattern 'import Skeleton from "$lib/shared/components/skeleton.svelte"'
Length of output: 164
Script:
#!/bin/bash
# Search for color configurations and skeleton usage with ripgrep
rg -l "colors\." --type-add 'svelte:*.svelte' --type svelte
rg -l "skeleton" --type-add 'svelte:*.svelte' --type svelte
# Check the actual implementation of the skeleton component
cat shared/components/skeleton.svelte
# Look for dashboard-related files that might use the skeleton
fd -e svelte dashboard
Length of output: 1780
Script:
#!/bin/bash
# Check dashboard-skeleton implementation and its color configuration
cat shared/components/dashboard-skeleton.svelte
# Check color usage in invoice dashboard views
cat packages/invoice-dashboard/src/lib/view-requests.svelte
cat packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Length of output: 52983
packages/invoice-dashboard/src/lib/view-requests.svelte (1)
548-554: 🛠️ Refactor suggestion
Optimize table sorting logic
The current implementation recalculates the sort column on every click, which could lead to inconsistent sorting behavior and unnecessary computations.
Consider memoizing the sort column or using a more deterministic approach:
- const sortBy = processedRequests?.some(
- (req) => req.payer?.value === signer
- )
- ? "payer.value"
- : "payee.value";
- handleSort(sortBy);
+ const sortBy = "type";
+ handleSort(sortBy);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/invoice-dashboard/src/lib/view-requests.svelte (2)
425-443: Enhance keyboard accessibility for search and column selectionThe search and column selection UI should be keyboard accessible.
Add keyboard event handlers:
<Input placeholder="Search..." width="w-[300px]" handleInput={handleSearchChange} + on:keydown={(e) => e.key === 'Enter' && handleSearchChange(e)} >
444-579: Add ARIA attributes for sorting functionalityThe table headers need proper ARIA attributes to indicate sortable columns and their current sort state.
Add ARIA attributes to the table headers:
<th on:click={() => handleSort("timestamp")}> <div> + <span role="columnheader" aria-sort={sortColumn === "timestamp" ? sortOrder : "none"}> Created<i class="caret"> {#if sortOrder === "asc" && sortColumn === "timestamp"} <ChevronUp /> {:else} <ChevronDown /> {/if} </i> + </span> </div> </th>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/invoice-dashboard/src/lib/view-requests.svelte(3 hunks)
🔇 Additional comments (5)
packages/invoice-dashboard/src/lib/view-requests.svelte (5)
15-15: LGTM: Import changes align with loading state improvements
The addition of the DashboardSkeleton component import aligns with the PR objective to improve loading indicators.
145-146: Redundant loading state management
This issue was previously identified in past reviews.
703-735: Pagination accessibility improvements needed
This issue was previously identified in past reviews.
655-674: Improve error handling in PDF export
This issue was previously identified in past reviews.
581-684: 🛠️ Refactor suggestion
Add error state handling for failed data fetches
The table only handles loading and empty states, but not error states.
Add error state handling:
- {#if !loading && processedRequests}
+ {#if error}
+ <tr>
+ <td colspan="100%">
+ <div class="error-state">
+ <p>Failed to load requests</p>
+ <button on:click={getRequests}>Retry</button>
+ </div>
+ </td>
+ </tr>
+ {:else if !loading && processedRequests}Likely invalid or redundant comment.
…ton' of github.com:RequestNetwork/web-components into 158-improve-invoice-dashboard-loading-indicator---skeleton
sstefdev
changed the title
aimensahnoun
deleted the
158-improve-invoice-dashboard-loading-indicator---skeleton
branch
Fixes #158
Problem
The current three loading bars in the Invoice Dashboard provide a suboptimal loading experience and do not effectively represent the rows and columns structure of the dashboard.
Changes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
DashboardSkeletoncomponent for improved loading UI.Improvements
Bug Fixes