chore(ui): consistent back navigation #1164
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
@iiio2 is attempting to deploy a commit to the serhalp's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds a new composable Possibly related PRs
🚥 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)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/pages/compare.vue (1)
118-123:⚠️ Potential issue | 🟡 MinorRemove per-button
focus-visibleutility class.The project uses a global focus-visible rule for buttons; keep this button consistent and rely on the global styling instead.
♻️ Suggested change
- class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"Based on learnings: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components.
app/pages/privacy.vue (1)
30-35:⚠️ Potential issue | 🟡 MinorRemove per-button
focus-visibleutility class.Use the global focus-visible styling for buttons rather than inline utilities on this button.
♻️ Suggested change
- class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"Based on learnings: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components.
app/pages/settings.vue (1)
52-56:⚠️ Potential issue | 🟡 MinorRemove per-button
focus-visibleutility class.Rely on the global button focus-visible rule for this Back button.
♻️ Suggested change
- class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0 p-1.5 -mx-1.5" + class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0 p-1.5 -mx-1.5"Based on learnings: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components.
app/pages/about.vue (1)
53-58:⚠️ Potential issue | 🟡 MinorRemove per-button
focus-visibleutility class.Stick to the global focus-visible styling for buttons to keep accessibility styling consistent.
♻️ Suggested change
- class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"Based on learnings: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components.
app/composables/useCanGoBack.ts
Outdated
| export function useCanGoBack() { | ||
| const canGoBack = ref(false) | ||
|
|
||
| if (import.meta.client) { | ||
| const router = useRouter() | ||
| canGoBack.value = router.options.history.state.back !== null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vue Router 4 router.options.history.state undefined null documentation
💡 Result:
In Vue Router 4, the history implementation you pass to createRouter({ history }) implements RouterHistory, which has a state: HistoryState property (documented, but marked Alpha in the API docs). (router.vuejs.org)
Why router.options.history.state can be undefined / null
router.optionsis not the recommended public surface to read from. Even though it exists, it’s not the API the docs point you to for “history state”.- Browser
window.history.statecan benull(e.g., before anything has calledpushState/replaceState, or depending on how the entry was created). Vue Router also stores its own navigation info inhistory.state, and the docs warn you to preserve/merge it if you ever callhistory.replaceState()yourself. (typeerror.org)
What the documentation does recommend using
- If your goal is to attach/read state for navigations, use the documented
stateoption in navigation:router.push({ ..., state: { ... } })/router.replace({ ..., state: { ... } })(router.vuejs.org)
- Vue Router’s direction (per core discussion) is to expose this as
route.state(reactive), rather than having people poke atrouter.options.history.stateor rawwindow.history.state. (github.com)
Practical guidance
- Don’t rely on
router.options.history.statefor app logic. - Use navigation
state(router.push/replace) and (where available in your version) read it fromroute.state; otherwise, readwindow.history.statebut assume it may benulland that Vue Router also uses it internally. (typeerror.org)
Citations:
- 1: https://router.vuejs.org/api/interfaces/routerhistory?utm_source=openai
- 2: https://www.typeerror.org/docs/vue_router~4/guide/migration/index?utm_source=openai
- 3: https://router.vuejs.org/api/interfaces/routelocationoptions.html?utm_source=openai
- 4: Allow changing `history.state` with `router.push()` vuejs/rfcs#400
- 5: https://www.typeerror.org/docs/vue_router~4/guide/migration/index?utm_source=openai
🏁 Script executed:
find . -name "useCanGoBack.ts" -type fRepository: npmx-dev/npmx.dev
Length of output: 94
🏁 Script executed:
cat -n ./app/composables/useCanGoBack.tsRepository: npmx-dev/npmx.dev
Length of output: 345
🏁 Script executed:
rg "useCanGoBack" --type ts --type tsx -A 3 -B 3Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
rg "useCanGoBack" -A 3 -B 3Repository: npmx-dev/npmx.dev
Length of output: 1595
Use the browser History API or Vue Router's navigation state; router.options.history.state is not a documented public API and accessing .back is unsafe.
router.options.history.state is an internal, undocumented property and not recommended. The proper approach in Vue Router 4 is to either:
- Use
window.history.statedirectly (assuming it may benull), or - Use the
stateoption inrouter.push()/router.replace()and read fromroute.state
Beyond the API concern, the current code accesses .back without guarding against undefined or null, which will fail if the browser history state is not initialised. If you must inspect window.history.state, use optional chaining: Boolean(window.history.state?.back).
danielroe
changed the title
|
Thanks @danielroe . 😊 |
2 participants
this actually related to this PR #1133
Current we've implemented back navigation only when it is necessary. When we go to any page like https://npmx.dev/about or https://npmx.dev/compare, we firstly see the back button then it disappears - a flash of the back button before it disappears. So, I think it is like a UX thing. So, that's why I've tried to improve it.