refactor: create a backbutton component#2024
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! |
📝 WalkthroughWalkthroughThis PR adds a new Vue 3 BackButton component that uses useRouter() and useCanGoBack() to render a conditional back-navigation button and calls router.back() on click. Several pages (About, Accessibility, Compare, PDS, Privacy, Recharging, Settings) are refactored to replace their inline back-button markup and router logic with the BackButton component. Tests are updated: an accessibility test for BackButton is added and test utilities are modified to normalise component file paths. 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)
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.
🧹 Nitpick comments (1)
app/components/BackButton.vue (1)
16-23: Remove inlinefocus-visibleutility; rely on global CSS.The project applies focus-visible styling for buttons globally via
main.css. The inlinefocus-visible:outline-accent/70class duplicates and potentially conflicts with the global rule.♻️ Suggested fix
<button v-if="canGoBack" type="button" :class="[ - 'inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0', + 'inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0', $props.class, ]" `@click`="router.back()" >Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91e60ec1-f868-4cd9-9d32-ba1262534a48
📒 Files selected for processing (9)
app/components/BackButton.vueapp/pages/about.vueapp/pages/accessibility.vueapp/pages/blog/index.vueapp/pages/compare.vueapp/pages/pds.vueapp/pages/privacy.vueapp/pages/recharging.vueapp/pages/settings.vue
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: fe22398a-fc3d-44f3-b834-4cce13851bd3
📒 Files selected for processing (2)
test/nuxt/a11y.spec.tstest/unit/a11y-component-coverage.spec.ts
|
Hi! I initially removed the back button because it wasn't clear where you were always going back to, and I figured a browser back button suffices? In my opinion the blog page is a separate kind of area to the rest of the app and doesn't necessarily need to replicate all functionality. Additionally, I find the back button on the right (for left to right languages) also quite confusing. I'm happy to be proved wrong by other reviewers though. |
I think currently the back button only shows up when you've navigated to a page that has it, then it will take you back to the page you navigated from. Considering that I'm for this change, in lieu of a discussion about removing the back button from all pages that is - WDYT? |
|
I think given we want to try and make the UI as clean and uncluttered as possible, I think removing all back buttons is a good call, especially as each page has it's own URL anyway. |
|
Oki let's for now not make any changes to where has/has not a back button in lieu of a wider discussion. We can take the core part of this pr adding the BackButton component to reduce duplication - ty @epifaniofrancisco |
|
Thanks for your first contribution, @epifaniofrancisco! 🙌 We'd love to welcome you to the npmx community. Come and say hi on Discord! And once you've joined, visit npmx.wamellow.com to claim the contributor role. |
🔗 Linked issue
🧭 Context
The Blog page didn’t include the same “back” navigation that exists on other informational pages (Privacy, Accessibility, Settings, etc.), causing inconsistent UX when navigating to/blog.This PR fixes
the Blog pageand standardizes the back button implementation across the app.📚 Description
<BackButton />component:Updated the Blog page header (app/pages/blog/index.vue) to include<BackButton /><BackButton />:app/pages/accessibility.vueapp/pages/compare.vueapp/pages/pds.vueapp/pages/privacy.vueapp/pages/recharging.vueapp/pages/settings.vue