fix(ui): enlarge chart skeleton title block#2108
fix(ui): enlarge chart skeleton title block#2108Kemalau wants to merge 1 commit intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughUpdates FacetBarChart.vue skeleton layout in both fallback and v-else branches: introduces a grouped header skeleton (two lines sized h-5 w-36 and h-4 w-52) within a container using gap-3 and a data-test="facet-bar-chart-skeleton" attribute. Replaces a single v-for block (h-7 w-full) with per-package skeleton rows sized h-8 w-full. Both branches now share the same outer container structure. Adds a new test FacetBarChart.spec.ts that mounts the component with facetLoading, asserts the skeleton container exists, verifies four SkeletonInline instances, and checks their heights: h-5, h-4, h-8, h-8. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/Compare/FacetBarChart.vue (1)
286-294: Consider extracting duplicate skeleton markup.The skeleton structure is duplicated between the
#fallbackslot (lines 286-294) and thev-elseblock (lines 299-307). While this works correctly and the duplication is small, you could extract it into a local template ref or a small helper component to ensure both remain in sync if future changes are needed.♻️ Optional: Extract skeleton to a named slot or local component
+<template `#skeleton`> + <div class="flex flex-col gap-3" data-test="facet-bar-chart-skeleton"> + <div class="flex flex-col items-center gap-2 mb-3"> + <SkeletonInline class="h-5 w-36 max-w-full" /> + <SkeletonInline class="h-4 w-52 max-w-full" /> + </div> + <div class="flex flex-col gap-2"> + <SkeletonInline class="h-8 w-full" v-for="pkg in packages" :key="pkg" /> + </div> + </div> +</template>Then reference it in both locations. Alternatively, keep as-is if the duplication is intentional for clarity.
Also applies to: 299-307
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bb793f0-08be-44f2-84d9-7ecc0089a97b
📒 Files selected for processing (2)
app/components/Compare/FacetBarChart.vuetest/nuxt/components/compare/FacetBarChart.spec.ts
Kemalau
changed the title
graphieros
left a comment
graphieros
left a comment
There was a problem hiding this comment.
I think the height of the skeleton bars for the chart section should be more dynamic.
There is still a size difference on desktop:
But more importantly, a fixed height for skeleton bars will not match on mobile.
| const skeleton = component.find('[data-test="facet-bar-chart-skeleton"]') | ||
| expect(skeleton.exists()).toBe(true) | ||
|
|
||
| const lines = skeleton.findAllComponents({ name: 'SkeletonInline' }) |
There was a problem hiding this comment.
checks for individual heights of placeholder items is not very useful. if we changed the height of the title to 8px for example, we'd have to update all of the unit tests for this. let's instead focus on the input instead and making sure that it exists. (eg:
testPackages = ['react', 'vue']
props: { ..... etc etc
...packages: testPackages
const lines = skeleton.findAllComponents({ name: 'SkeletonInline' })
expect(lines).toHaveLength(2 + testPackages.length)
| const lines = skeleton.findAllComponents({ name: 'SkeletonInline' }) | ||
| expect(lines).toHaveLength(4) | ||
| expect(lines[0]?.attributes('class')).toContain('h-5') | ||
| expect(lines[1]?.attributes('class')).toContain('h-4') |
There was a problem hiding this comment.
not a blocker for this PR, but this spec file could benefit from more comprehensive unit tests. success state, empty state, testing dynamic updates like the watch on props.packages etc
There was a problem hiding this comment.
This is indeed not really in the scope of this issue, but can be part of an iteration pertaining to test coverage improvements.
| </div> | ||
| <div class="flex flex-col gap-1"> | ||
| <SkeletonInline class="h-7 w-full" v-for="pkg in packages" :key="pkg" /> | ||
| <div class="flex flex-col gap-3" data-test="facet-bar-chart-skeleton"> |
There was a problem hiding this comment.
we're repeating the same element twice. could we extract this into its own separate component and reuse in multiple places?
There was a problem hiding this comment.
Repeating just twice, especially when only localised to this file only, is ok.
3 participants
Why
The compare page bar-chart skeleton was shorter than the rendered chart title/subtitle block, which caused layout shift when datapoints loaded.
What changed
FacetBarChartVerification
pnpm exec oxlint app/components/Compare/FacetBarChart.vue test/nuxt/components/compare/FacetBarChart.spec.tspnpm exec oxfmt --check app/components/Compare/FacetBarChart.vue test/nuxt/components/compare/FacetBarChart.spec.tspnpm test test/nuxt/components/compare/FacetBarChart.spec.ts(blocked in this environment: Playwright browser launch fails becauselibatk-1.0.so.0is missing on the host)Closes #2106.