Uh oh!
There was an error while loading. Please reload this page.
refactor: move package-header to new component - #2030
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request extracts the package page header into a new Vue 3 component at Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b2f1f42-533d-4182-aa80-4a05674df696
📒 Files selected for processing (3)
app/components/Package/Header.vueapp/composables/usePackageHeaderHeight.tsapp/pages/package/[[org]]/[name].vue
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/nuxt/a11y.spec.ts (1)
729-757: The new a11y case skips the moved action states.With
docsLink/codeLinkset tonullandlatestVersionmatchingdisplayVersion, this never renders the header actions that were moved out of the page. A regression in those links/buttons would still pass the suite.Suggested follow-up
describe('PackageHeader', () => { + const baseProps = {+ pkg: { name: 'vue' },+ resolvedVersion: '3.5.0',+ displayVersion: {+ _id: '1234567890',+ _npmVersion: '3.5.0',+ name: 'vue',+ version: '3.5.0',+ dist: {+ shasum: '1234567890',+ signatures: [],+ tarball: 'https://npmx.dev/package/vue/tarball',+ },+ },+ latestVersion: { version: '3.5.0', tags: [] },+ provenanceData: null,+ provenanceStatus: 'idle',+ docsLink: null,+ codeLink: null,+ isBinaryOnly: false,+ }+ it('should have no accessibility violations', async () => { const component = await mountSuspended(PackageHeader, { - props: {- pkg: { name: 'vue' },- resolvedVersion: '3.5.0',- displayVersion: {- _id: '1234567890',- _npmVersion: '3.5.0',- name: 'vue',- version: '3.5.0',- dist: {- shasum: '1234567890',- signatures: [],- tarball: 'https://npmx.dev/package/vue/tarball',- },- },- latestVersion: { version: '3.5.0', tags: [] },- provenanceData: null,- provenanceStatus: 'idle',- docsLink: null,- codeLink: null,- isBinaryOnly: false,- },+ props: baseProps, }) const results = await runAxe(component) expect(results.violations).toEqual([]) }) ++ it('should have no accessibility violations with header actions visible', async () => {+ const component = await mountSuspended(PackageHeader, {+ props: {+ ...baseProps,+ latestVersion: { version: '3.6.0', tags: ['latest'] },+ docsLink: { name: 'docs', params: { path: ['vue', 'v', '3.5.0'] } },+ codeLink: {+ name: 'code',+ params: { packageName: 'vue', version: '3.5.0', filePath: '' },+ },+ },+ })+ const results = await runAxe(component)+ expect(results.violations).toEqual([])+ }) })As per coding guidelines,
**/*.{test,spec}.{ts,tsx}: Write unit tests for core functionality usingvitest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6512c139-fb07-4673-97ec-6fffbd7b5d60
📒 Files selected for processing (2)
app/pages/package/[[org]]/[name].vuetest/nuxt/a11y.spec.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
🧭 Context
I'm working on the header updating to the new UI. To avoid making too big PR, I'm currently creating a separate PR to move the header into a separate component
I'll create a prs train next, but we can merge this part right away to make things easier for others and to reduce conflicts
Essentially, it's almost just a move. The only interesting part is package-header height calculations via global state, making it easier to share between components (I'm thinking of converting to CSS-variable later, but for now, this is better and closer to original approach)