feat: Skeletons cards + Format #85
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces loading skeletons for skills, projects, and talks across multiple pages. Adds parallel profile/projects loading in ProjectsPage. Updates project and talk card skeleton components and styles, including a new CSS class. Adjusts tests for new loading behavior. Updates Makefile and ESLint configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant P as ProjectsPage
participant API as API
Note over P: onMounted
P->>P: isLoadingProfile = true<br/>isLoadingProjects = true
par Load profile
P->>API: getProfile()
API-->>P: profile or error
P->>P: finally { isLoadingProfile = false }
and Load projects
P->>API: getProjects()
API-->>P: projects or error
P->>P: finally { isLoadingProjects = false }
end
alt Loading or empty
P-->>U: Render skeletons (skills/projects)
else Data available
P-->>U: Render skills and project cards
end
sequenceDiagram
autonumber
participant U as User
participant T as TalksPartial
participant API as API
Note over T: onMounted
T->>T: isLoadingTalks = true
T->>API: getTalks()
API-->>T: talks or error
T->>T: finally { isLoadingTalks = false }
alt Loading or empty
T-->>U: Show 4 TalkCardSkeletonPartial
else Has talks
T-->>U: Render talk cards
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Biome (2.1.2)src/css/style.css[error] 25-25: Unknown property is not allowed. See CSS Specifications and browser specific properties for more details. (lint/correctness/noUnknownProperty) 🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses layout instability on the projects page by standardizing the minimum height of project cards and their loading skeletons. It also improves the user experience by ensuring that the layout remains consistent and stable, even when no projects are available, by displaying skeleton placeholders rather than an abrupt empty state. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to stabilize the project page layout by ensuring project cards and their skeletons have a matching minimum height, and by displaying skeletons when no projects are available. The changes successfully implement this. My review includes suggestions to simplify some of the new template logic in ProjectsPage.vue, improve the user experience for the empty state, and address missing test coverage for the new behavior. I've also pointed out an opportunity to reduce code duplication by creating a shared CSS class for card styles.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/ProjectsPage.vue (1)
34-40: Unified conditional simplifies template logic.The consolidated rendering logic correctly shows skeletons during loading and renders project cards when data is available. However, two points to consider:
Simplify the second condition: Line 37's
v-else-if="projects.length > 0"is redundant. Since the firstv-ifalready handlesprojects.length === 0, the second template will only be reached whenprojects.length > 0. Simplify tov-else.Empty-state UX: Showing skeletons when
projects.length === 0(no projects returned) might confuse users who expect a message like "No projects available." Is this intentional to maintain layout consistency, or should there be a distinct empty-state message?Apply this diff to simplify the conditional:
- <template v-else-if="projects.length > 0"> + <template v-else> <ProjectCardPartial v-for="project in projects" :key="project.uuid" :item="project" /> </template>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/ProjectsPage.vue(1 hunks)src/partials/ProjectCardPartial.vue(1 hunks)src/partials/ProjectCardSkeletonPartial.vue(2 hunks)
🔇 Additional comments (2)
src/partials/ProjectCardPartial.vue (1)
9-9: LGTM! Minimum height ensures layout stability.Adding
min-h-[220px]to the card container aligns with the skeleton partial and prevents layout shift during loading/empty states.src/partials/ProjectCardSkeletonPartial.vue (1)
3-3: LGTM! Skeleton now matches card minimum height.Adding
min-h-[220px]ensures the skeleton placeholder maintains the same dimensions as the rendered project card, preventing layout shift.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/pages/ProjectsPage.vue (1)
34-44: Restore an explicit empty-state messageWhen the fetch resolves with zero projects, we now leave people with four inert skeletons and no text. That’s a regression from the previous “No projects are available…” copy and leaves screen-reader users with silence. Please keep the skeletons for layout stability but present a real empty-state message once loading finishes. For example:
<template v-if="isLoadingProjects || projects.length === 0"> <ProjectCardSkeletonPartial v-for="index in 4" :key="`projects-page-skeleton-${index}`" :is-animated="isLoadingProjects" /> + <p + v-if="!isLoadingProjects && projects.length === 0" + class="text-sm text-slate-500 mt-3" + > + No projects are available right now. Please check back soon. + </p> </template>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/css/support/utility-patterns.css(1 hunks)src/pages/AboutPage.vue(4 hunks)src/pages/HomePage.vue(4 hunks)src/pages/ProjectsPage.vue(5 hunks)src/pages/ResumePage.vue(4 hunks)src/partials/ProjectCardPartial.vue(1 hunks)src/partials/ProjectCardSkeletonPartial.vue(2 hunks)src/partials/TalkCardSkeletonPartial.vue(1 hunks)src/partials/TalksPartial.vue(2 hunks)src/partials/WidgetSkillsSkeletonPartial.vue(1 hunks)tests/pages/ProjectsPage.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/partials/WidgetSkillsSkeletonPartial.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src/partials/ProjectCardSkeletonPartial.vue
🧰 Additional context used
🧬 Code graph analysis (1)
tests/pages/ProjectsPage.test.ts (1)
src/stores/api/store.ts (2)
getProfile(42-50)getProjects(69-77)
🪛 GitHub Actions: Unit Tests
tests/pages/ProjectsPage.test.ts
[error] 97-97: AssertionError: expected [ 'rounded-lg', 'border', …(7) ] to not include 'animate-pulse'
🪛 GitHub Check: vitest
tests/pages/ProjectsPage.test.ts
[failure] 97-97: tests/pages/ProjectsPage.test.ts > ProjectsPage > renders static skeletons when no projects are returned
AssertionError: expected [ 'rounded-lg', 'border', …(7) ] to not include 'animate-pulse'
❯ tests/pages/ProjectsPage.test.ts:97:56
❯ tests/pages/ProjectsPage.test.ts:96:27
🔇 Additional comments (4)
tests/pages/ProjectsPage.test.ts (4)
3-6: LGTM!The new imports support the test structure refactoring (
beforeEach) and the skeleton rendering test (ProjectCardSkeletonPartial).
40-49: LGTM!The
beforeEachhook properly resets and configures mocks for test isolation, and the mock return values match the expected API response structure.
55-74: LGTM!The test improvements include a more realistic
ProjectCardPartialstub that renders actual project titles, and the addition ofFooterPartialto the stubs aligns with the component's layout structure.
101-119: LGTM!The error handling test correctly verifies that API failures are properly handled and logged via
debugError.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/ProjectsPage.vue (1)
118-130: Fixed: reset isLoadingProjects in finallyThis addresses the earlier test failure about animate-pulse persisting. Nice.
🧹 Nitpick comments (5)
src/partials/TalksPartial.vue (1)
7-9: Unify skeleton animation logic with ProjectsTo keep behavior consistent and avoid edge cases during refresh, pass only the loading flag.
- <TalkCardSkeletonPartial v-for="index in 4" :key="`talk-skeleton-${index}`" :is-animated="isLoadingTalks && talks.length === 0" /> + <TalkCardSkeletonPartial v-for="index in 4" :key="`talk-skeleton-${index}`" :is-animated="isLoadingTalks" />src/pages/AboutPage.vue (1)
84-147: Consider extracting a reusable profile loaderProfile fetch + isLoadingProfile pattern repeats across pages; a small useProfile composable would reduce duplication.
src/pages/ProjectsPage.vue (1)
34-39: Correct empty/loading skeleton behavior; add a11y hint (optional)Showing skeletons when loading or empty is correct; consider an sr-only message for screen readers when not loading and empty.
<template v-if="isLoadingProjects || projects.length === 0"> <ProjectCardSkeletonPartial v-for="index in 4" :key="`projects-page-skeleton-${index}`" :is-animated="isLoadingProjects" /> + <div v-if="!isLoadingProjects" class="sr-only">No projects found</div> </template>src/partials/ProjectCardSkeletonPartial.vue (2)
2-5: Respect prefers-reduced-motion and hide decorative skeletons from screen readersUse motion-safe variant and mark the skeleton container aria-hidden. Keeps animation off for users who prefer reduced motion and avoids SR noise.
- <div - class="rounded-lg border border-slate-200 dark:border-slate-800 dark:bg-gradient-to-t dark:from-slate-800 dark:to-slate-800/30 p-5" - :class="[{ 'animate-pulse': props.isAnimated }, props.wrapperClass]" - > + <div + class="rounded-lg border border-slate-200 dark:border-slate-800 dark:bg-gradient-to-t dark:from-slate-800 dark:to-slate-800/30 p-5" + :class="[{ 'motion-safe:animate-pulse': props.isAnimated }, props.wrapperClass]" + aria-hidden="true" + >
27-35: Name the component; broadenwrapperClasstyping; consider deprecatingwrapperClassin favor of native class mergingVue auto-merges class attrs onto the root, so
wrapperClasscan be optional long‑term. Meanwhile, add a component name and widen typing.+defineOptions({ name: 'ProjectCardSkeletonPartial' }); const props = withDefaults( defineProps<{ - wrapperClass?: string; + wrapperClass?: string | string[] | Record<string, boolean>; isAnimated?: boolean; }>(), { isAnimated: true, }, );Optional next step: deprecate
wrapperClassand rely on passingclassdirectly to the component; remove the prop in a future major.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/pages/AboutPage.vue(4 hunks)src/pages/HomePage.vue(4 hunks)src/pages/ProjectsPage.vue(5 hunks)src/pages/ResumePage.vue(4 hunks)src/partials/ProjectCardPartial.vue(1 hunks)src/partials/ProjectCardSkeletonPartial.vue(2 hunks)src/partials/TalkCardSkeletonPartial.vue(1 hunks)src/partials/TalksPartial.vue(3 hunks)src/partials/WidgetSkillsSkeletonPartial.vue(1 hunks)tests/pages/ProjectsPage.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/partials/TalkCardSkeletonPartial.vue
- src/partials/WidgetSkillsSkeletonPartial.vue
- src/pages/ResumePage.vue
- src/partials/ProjectCardPartial.vue
🧰 Additional context used
🧬 Code graph analysis (1)
tests/pages/ProjectsPage.test.ts (1)
src/stores/api/store.ts (2)
getProfile(42-50)getProjects(69-77)
🔇 Additional comments (8)
src/partials/TalksPartial.vue (1)
50-67: Loading state wiring looks goodImporting the skeleton and resetting isLoadingTalks in finally is correct.
src/pages/AboutPage.vue (1)
71-73: Solid loading/empty-state handling for skillsConditional render with skeleton and finally-reset for isLoadingProfile is correct and consistent with the new pattern.
Also applies to: 143-145
tests/pages/ProjectsPage.test.ts (2)
77-101: Good coverage for static skeletons on empty projectsTest correctly ensures four skeletons render without animate-pulse after load completes; nextTick after flushPromises is appropriate.
119-119: Sync after promise resolutionAdding nextTick after flushPromises avoids timing flakiness; good call.
src/pages/HomePage.vue (1)
29-31: Skills skeleton + loading flag finalizedPattern matches other pages and correctly resets loading in finally.
Also applies to: 52-53, 62-63, 93-94
src/pages/ProjectsPage.vue (2)
104-116: Profile loader with finally is correctError is handled and the loading flag is cleared reliably.
132-134: Parallelized loading via Promise.allGood optimization and keeps UI responsive.
src/partials/ProjectCardSkeletonPartial.vue (1)
6-6:.project-card-contentutility verifiedIt’s defined with
min-h-[220px]insrc/css/support/utility-patterns.cssand applied insrc/partials/ProjectCardPartial.vue.
gocanto
changed the title
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68de411c80308333ba740de925adce62
Summary by CodeRabbit
New Features
Style
Tests
Chores