feat(components): base ScrollArea on RekaUI ScrollArea component#6140
feat(components): base ScrollArea on RekaUI ScrollArea component#6140mikenewbon wants to merge 2 commits intonuxt:v4from
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the previous primitive-based ScrollArea with reka-ui's ScrollAreaRoot, ScrollAreaViewport, ScrollAreaScrollbar, ScrollAreaThumb, and ScrollAreaCorner; adds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/runtime/components/ScrollArea.vue (1)
270-275: Consider extracting a single viewport getter for both exposed aliases.
$elandviewportcurrently duplicate the same lookup/cast. A shared getter would reduce drift risk.♻️ Optional cleanup
+const getViewportElement = () => rootRef.value?.viewport as HTMLElement | undefined + defineExpose({ get $el() { - return rootRef.value?.viewport as HTMLElement | undefined + return getViewportElement() }, get viewport() { - return rootRef.value?.viewport as HTMLElement | undefined + return getViewportElement() },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/ScrollArea.vue` around lines 270 - 275, Both $el and viewport duplicate the same lookup/cast from rootRef.value?.viewport; extract a single shared getter (e.g., a private computed or function like getViewport or viewportEl) that returns rootRef.value?.viewport as HTMLElement | undefined and have the existing getters $el and viewport simply return that single getter; update references in the component to use the new shared getter name and remove the duplicated casting logic from $el and viewport.docs/content/docs/2.components/scroll-area.md (1)
245-247: Includecornerin the UI customization note for completeness.This note currently mentions
scrollbarandthumb, butcorneris also now customizable in this PR.📝 Suggested doc tweak
- The scrollbar appearance can be customized via the `ui` prop using the `scrollbar` and `thumb` slots. + The scrollbar appearance can be customized via the `ui` prop using the `scrollbar`, `thumb`, and `corner` slots.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/2.components/scroll-area.md` around lines 245 - 247, Update the docs note about ScrollArea UI customization to list the new `corner` slot alongside `scrollbar` and `thumb`; specifically, edit the sentence that currently reads "The scrollbar appearance can be customized via the `ui` prop using the `scrollbar` and `thumb` slots." to include `corner` (e.g., "`scrollbar`, `thumb`, and `corner` slots") so the `ui` prop customization description and references to slots (scrollbar, thumb, corner) are accurate for the ScrollArea component.test/components/ScrollArea.spec.ts (1)
28-35: Add atype: 'glimpse'render case to complete the variant matrix.You already cover
auto/always/scroll/hover; addingglimpsewould align tests with the documented prop options.✅ Suggested test case
['with type auto', { props: { ...props, type: 'auto' } }], ['with type always', { props: { ...props, type: 'always' } }], ['with type scroll', { props: { ...props, type: 'scroll' } }], ['with type hover', { props: { ...props, type: 'hover' } }], + ['with type glimpse', { props: { ...props, type: 'glimpse' } }], ['with scrollHideDelay', { props: { ...props, type: 'scroll', scrollHideDelay: 1000 } }],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/ScrollArea.spec.ts` around lines 28 - 35, Add a new test case entry to the existing test cases array in ScrollArea.spec.ts to cover the missing "glimpse" variant: create an entry like ['with type glimpse', { props: { ...props, type: 'glimpse' } }] alongside the other type cases (the array that currently includes 'with type auto', 'with type always', 'with type scroll', 'with type hover'); ensure it uses the same props spread pattern and naming convention so the render matrix includes the documented type option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/content/docs/2.components/scroll-area.md`:
- Around line 245-247: Update the docs note about ScrollArea UI customization to
list the new `corner` slot alongside `scrollbar` and `thumb`; specifically, edit
the sentence that currently reads "The scrollbar appearance can be customized
via the `ui` prop using the `scrollbar` and `thumb` slots." to include `corner`
(e.g., "`scrollbar`, `thumb`, and `corner` slots") so the `ui` prop
customization description and references to slots (scrollbar, thumb, corner) are
accurate for the ScrollArea component.
In `@src/runtime/components/ScrollArea.vue`:
- Around line 270-275: Both $el and viewport duplicate the same lookup/cast from
rootRef.value?.viewport; extract a single shared getter (e.g., a private
computed or function like getViewport or viewportEl) that returns
rootRef.value?.viewport as HTMLElement | undefined and have the existing getters
$el and viewport simply return that single getter; update references in the
component to use the new shared getter name and remove the duplicated casting
logic from $el and viewport.
In `@test/components/ScrollArea.spec.ts`:
- Around line 28-35: Add a new test case entry to the existing test cases array
in ScrollArea.spec.ts to cover the missing "glimpse" variant: create an entry
like ['with type glimpse', { props: { ...props, type: 'glimpse' } }] alongside
the other type cases (the array that currently includes 'with type auto', 'with
type always', 'with type scroll', 'with type hover'); ensure it uses the same
props spread pattern and naming convention so the render matrix includes the
documented type option.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/ScrollArea-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/ScrollArea.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
docs/content/docs/2.components/scroll-area.mdsrc/runtime/components/ScrollArea.vuesrc/theme/scroll-area.tstest/components/ScrollArea.spec.ts
commit: |
|
@mikenewbon I'm not sure this is the right move, the Reka UI ScrollArea component is meant to augment native scroll functionality. This is not what we're doing here 🤔 |
|
@benjamincanac This suddenly jumped on my priority list after seeing my app on a windows laptop and wanting nice scrollbars. Basing this off Reka seemed like a double win for accessibility. Could also look at implementing it myself, wdyt? |
2 participants
#6139
❓ Type of change
📚 Description
As mentioned here, we should base ScrollArea off the Reka example for proper accessibility and styling on the scroll/thumb.
#5201 (comment)
📝 Checklist