feat(web): add loading and error states to notification sidebar #974
Conversation
WalkthroughThis pull request introduces four new Vue components: Changes
Possibly related PRs
Suggested reviewers
🪧 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: 3
🧹 Outside diff range and nitpick comments (5)
web/components/Loading/Bar.vue (1)
4-4: Consider adding prop validation for class typeWhile TypeScript provides type safety, consider adding runtime validation to ensure the class prop is a string.
-const props = withDefaults(defineProps<{ class?: string }>(), { class: '' }); +const props = withDefaults( + defineProps<{ + /** Additional classes to apply to the loading bar */ + class?: string; + }>(), + { class: '' } +);web/components/Loading/Spinner.vue (2)
4-4: Add documentation for the class propAdd JSDoc comments to document the prop's purpose and usage.
+/** + * Props for the Spinner component + * @property {string} class - Additional classes to apply to the spinner + */ const props = withDefaults(defineProps<{ class?: string }>(), { class: '' });
9-16: Consider extracting default classes to a constantThe long class string could be moved to a constant for better maintainability.
+const DEFAULT_SPINNER_CLASSES = + 'inline-block h-8 w-8 animate-spin rounded-full border-2 border-solid ' + + 'border-current border-e-transparent align-[-0.125em] text-primary ' + + 'motion-reduce:animate-[spin_1.5s_linear_infinite]'; <div :class=" cn( - 'inline-block h-8 w-8 animate-spin rounded-full border-2 border-solid border-current border-e-transparent align-[-0.125em] text-primary motion-reduce:animate-[spin_1.5s_linear_infinite]', + DEFAULT_SPINNER_CLASSES, props.class ) )" role="status" >web/components/Notifications/List.vue (2)
83-85: Consider using LoadingBar for infinite scrollThe spinner might be visually disruptive when loading more items. Consider using LoadingBar for a smoother infinite scroll experience.
- <div v-if="loading" class="py-5 grid place-content-center"> - <LoadingSpinner /> - </div> + <LoadingBar v-if="loading" class="my-5 mx-auto w-1/2" />
104-106: Improve empty state message formattingThe empty state message concatenation could be improved for better readability and maintainability.
- {{ `No ${props.importance?.toLowerCase() ?? ''} notifications to see here!` }} + <p class="text-gray-500"> + No{{ props.importance ? ` ${props.importance.toLowerCase()}` : '' }} notifications to see here! + </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/components/Loading/Bar.vue(1 hunks)web/components/Loading/Spinner.vue(1 hunks)web/components/Notifications/List.vue(3 hunks)
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
pujitm
force-pushed
the
feat/notif-error-state
branch
from
50e24d9 to
027806a
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/components/Loading/Error.vue (2)
25-26: Fix typo in prop documentationThe comment "hasdfsa" appears to be a typo or placeholder for the
loadingprop documentation.- /** hasdfsa */ + /** Indicates whether content is currently loading */ loading: boolean;
37-40: Make loading message configurableThe loading message is hardcoded to "Loading Notifications...", which limits component reusability. Consider adding a prop for the loading message.
const props = withDefaults( defineProps<{ class?: string; loading: boolean; error: Error | null | undefined; + loadingMessage?: string; }>(), - { class: '' } + { + class: '', + loadingMessage: 'Loading...' + } ); <div v-if="loading" class="contents"> <LoadingSpinner /> - <p>Loading Notifications...</p> + <p>{{ props.loadingMessage }}</p> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/components/Loading/Error.vue(1 hunks)web/components/Notifications/List.vue(2 hunks)
🧰 Additional context used
📓 Learnings (1)
web/components/Notifications/List.vue (1)
Learnt from: pujitm
PR: unraid/api#974
File: web/components/Notifications/List.vue:31-31
Timestamp: 2024-12-06T14:43:04.876Z
Learning: In the Vue.js application using `useQuery` from Vue Apollo, the `error` reactive variable captures errors from both the initial query and any `fetchMore` calls, so wrapping `fetchMore` in a try-catch block is unnecessary.
🔇 Additional comments (4)
web/components/Loading/Error.vue (1)
53-53: LGTM!
The default state implementation using slots is clean and follows Vue best practices.
web/components/Notifications/List.vue (3)
31-31: LGTM!
The addition of loading and refetch to the useQuery destructuring aligns with Vue Apollo best practices. As noted in the past learning, error handling for both initial query and fetchMore is handled by the error reactive variable.
83-85: LGTM! Loading state for infinite scroll
The second loading spinner is correctly implemented for infinite scroll pagination, while the main loading state in LoadingError handles the initial load.
88-93: LGTM! Clean implementation of loading and error states
The integration of LoadingError component effectively handles loading, error, and empty states, following the suggested approach from previous discussions. The implementation is clean and reusable.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
pujitm
force-pushed
the
feat/notif-error-state
branch
from
8b4c065 to
358495e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
web/components/Loading/Error.vue (3)
5-21: Enhance component documentationThe documentation is good but could be improved by adding descriptions for props and emits.
/** * A default container for displaying loading and error states. * * By default, this component will expand to full height and display contents * in the center of the container. * * Any slot/child will only render when a loading/error state isn't displayed. * * Exposes a 'retry' event (user-triggered during error state). * + * @prop {string} [class] - Additional CSS classes to apply to the container + * @prop {boolean} loading - Whether the component is in a loading state + * @prop {Error | null | undefined} error - Error object to display, if any + * @emits {void} retry - Emitted when the retry button is clicked during error state * * @example * <LoadingError @retry="retryFunction" :loading="loading" :error="error" /> * * <LoadingError :loading="loading" :error="error"> * <p>Only displayed when both loading and error are false.</p> * </LoadingError> */
25-25: Remove invalid commentThe comment "hasdfsa" appears to be unintentional.
- /** hasdfsa */
22-30: Add prop validation for error handlingConsider adding runtime validation for the error prop to ensure it's a proper Error object when provided.
const props = withDefaults( defineProps<{ class?: string; loading: boolean; error: Error | null | undefined; }>(), - { class: '' } + { + class: '', + validator: { + error: (value: Error | null | undefined) => { + return value === null || value === undefined || value instanceof Error; + } + } + } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/components/Loading/Bar.vue(1 hunks)web/components/Loading/Error.vue(1 hunks)web/components/Loading/Spinner.vue(1 hunks)web/components/Notifications/List.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/components/Loading/Bar.vue
- web/components/Loading/Spinner.vue
- web/components/Notifications/List.vue
🧰 Additional context used
📓 Learnings (1)
web/components/Loading/Error.vue (1)
Learnt from: pujitm
PR: unraid/api#974
File: web/components/Loading/Error.vue:50-50
Timestamp: 2024-12-06T17:34:16.133Z
Learning: In this project, the `Button` component from `~/components/shadcn/Button.vue` is autoloaded and does not need to be imported manually in components like `web/components/Loading/Error.vue`.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes