fix: debounce search results & centralized live region#1812
fix: debounce search results & centralized live region#1812danielroe merged 2 commits 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! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a debounced live region announcement system to the search page for screen readers. Introduces computed properties for raw and debounced messages, two debounced updater functions with different delays (desktop ≈250ms, mobile ≈700ms), a watcher that selects the appropriate debouncer by viewport, and lifecycle cleanup to cancel pending debounced updates on unmount. Removes several 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 (2)
app/pages/search.vue (2)
558-649: Well-structured live region debouncing implementation.The logic correctly coordinates mobile/desktop debouncing with proper cancellation of the opposite debounce when switching device types. Cleanup on unmount prevents memory leaks.
Minor observation: On line 579,
$n(effectiveTotal.value)already returns a formatted string, so the subsequent.toString()call (line 584) is redundant.Optional: Remove redundant toString()
const pSize = preferredPageSize.value === 'all' ? $n(effectiveTotal.value) : Math.min(preferredPageSize.value, effectiveTotal.value) return $t( 'filters.count.showing_paginated', { - pageSize: pSize.toString(), + pageSize: String(pSize), count: $n(effectiveTotal.value), }, effectiveTotal.value, ),
860-863: Centralized live region correctly implemented.The single
role="status"element at the end of the template provides a clean centralised announcement point for screen readers.Consider adding
aria-atomic="true"to ensure the entire message is announced when updated, rather than just the changed portion:Optional enhancement
- <div role="status" class="sr-only"> + <div role="status" aria-atomic="true" class="sr-only"> {{ debouncedLiveRegionMessage }} </div>,
041eea7 to
696dd0c
Compare
696dd0c to
a127bc3
Compare
knowler
left a comment
There was a problem hiding this comment.
This is great—thank you! I was hoping a solution would use a single live region, so thank you for doing that.
I’ll let someone with more Vue experience review the code, but if that’s fine, this is good to merge.
|
thank you very much for review! |
|
when the next nuxt version is out we might be able to use nuxt/nuxt#34318. |
Okay, I will continue to follow up. |
🔗 Linked issue
resolves #1803
🧭 Context
📚 Description
Centralized Live Region: Removed the
role="status"tags scattered throughout the user interface. Now, all content is handled by a singlesr-onlyelement at the bottom.Debouncing Updates: Added debouncing for live region (250ms on desktop, 700ms on mobile).
Less Noise: Hidden the "Updating..." message from screen readers