feat: unify loading states across the application#497
Conversation
✅ Deploy Preview for otel-ecosystem-explorer ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
12b1fe5 to
c54150f
Compare
|
Hi @SurbhiAgarwal1, thanks for the PR. The diff also includes all the changes from your PR #380 (README support). I suspect this branch was started from Could you rebase on an up-to-date |
9ef47a0 to
f621020
Compare
|
Hi @lucacavenaghi97, the branch is now fully cleaned up, rebased on main, and all CI checks are passing! Let me know if you need anything else for the review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
ecosystem-explorer/src/components/ui/Loader.tsx:32
- This component-level comment repeats the component name/purpose without adding a non-obvious invariant or workaround. Frontend guidance for this repo asks new explorer code to ship without comments that simply restate what the code already expresses.
/**
* A unified Loader component for consistent loading states across the application.
ecosystem-explorer/src/components/ui/Loader.tsx:57
- This inline comment just labels the immediately following glow markup. It does not explain a non-obvious invariant or workaround, so it should be removed under the frontend comment guidance for new explorer code.
{/* Glow effect */}
ecosystem-explorer/src/components/ui/Loader.tsx:28
- These comments restate the prop names rather than documenting non-obvious behavior. Per the frontend guidance, new comments should explain a specific invariant, footgun, or workaround, so these should be removed unless there is hidden behavior to document.
/** Optional additional CSS classes */
className?: string;
/** Optional text to display alongside the loader */
label?: string;
| export function Loader({ size = "lg", className = "", label }: LoaderProps) { | ||
| if (size === "sm") { |
| /** | ||
| * A unified Loader component for consistent loading states across the application. | ||
| */ | ||
| export function Loader({ size = "lg", className = "", label }: LoaderProps) { |
| /** | ||
| * Size of the loader. | ||
| * "lg" is for full-page states and Suspense fallbacks. | ||
| * "sm" is for inline/compact states. | ||
| */ |
| <div className="text-muted-foreground mb-4 text-sm"> | ||
| {isLoading | ||
| ? "Loading..." | ||
| ? "..." |
Standardizes the application's loading experience by replacing scattered, duplicated loading UI implementations with a single, reusable Loader component. Replaces manual loading blocks and plain text messages in instrumentation and collector pages, and unifies Suspense fallbacks. Resolved conflicts with upstream/main incorporating latest retry logic and SPA fallback protections.
f621020 to
877ba23
Compare
|
Hi @SurbhiAgarwal1, a few things still open before I can finish the review. 1. CI is red.
2. 3. 4. Red flash on Tested this fix locally and the flash is gone (in both - {!schemaVersion || schema.loading || starter.loading ? (
- <Loader size="sm" label="Loading schema…" className="mt-4" />
- ) : schema.error || !root ? (
+ {!schemaVersion || schema.loading || starter.loading || (!schema.error && !root) ? (
+ <Loader
+ size={root ? "sm" : "lg"}
+ label="Loading schema…"
+ className={root ? "mt-4" : undefined}
+ />
+ ) : schema.error ? (
<p className="mt-4 text-sm text-red-400">Failed to load schema.</p>Side benefit: |
Description
This Pull Request standardizes the application's loading experience by replacing scattered, duplicated loading UI implementations with a single, reusable
Loadercomponent. This ensures visual consistency across all feature pages and improves the overall user experience and accessibility.Fixes #486
Key Changes
1. New Unified Loader Component
src/components/ui/Loader.tsxas the single source of truth for loading states.lgvariant: Full-page centered loader with premium pulsing glow effects, secondary shadows, and a largeLoader2icon.smvariant: Inline compact loader for section-level or component-specific loading states.role="status",aria-live="polite", andaria-busy="true"for screen-reader compatibility.2. Comprehensive Refactoring
Replaced four inconsistent loading implementations (glow blocks, dashed boxes, plain text, and ad-hoc Suspense fallbacks) in the following areas:
Loader.Suspensefallbacks inLegacyAppandV1Appfor consistent route transitions.3. Conflict Resolution & Stability
upstream/main, incorporating the latest retry logic and SPA fallback protections.Verification
tsc -bpassed.eslint .passed.