-
-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ScrollArea] Create new ScrollArea
component
#665
Conversation
Netlify deploy preview |
It's about mui/mui-x#510, right? I have added the reference so developers can find a clear cross-link: We could envision this component under the Base UI X brand umbrella in the future because most applications don't have a custom scrollbar. It seems more niche relative to nailing a menu, or combo box use case. Now, it's doesn't match with the criteria for Base UI X we defined (>=4 pages, >=1-2 person full time), still, I find it interesting that it shows the potential opportunity cost of having it in the core, or even open-source. |
No we didn't see this issue prior to planning this work. It's just a basic building block of high-quality web dev, so I had planned to add it from the beginning.
Very many web apps (Slack, Linear, Radix docs etc. etc.) have custom scrollbars. It's not possible to build a high-quality web UI without them.
Do you think this belongs under the premium components? We need it for our new docs (for the sidebar and code blocks). It's not a huge investment, and Radix provides one for free. |
89e5913
to
42aee52
Compare
@colmtuite In any way, I mostly wanted to create a backlink between these two so it's easier to discover the link.
For peak execution, I very much agree. I think to be careful about the opportunity cost: downloads https://npm-stat.com/charts.html?package=overlayscrollbars&package=react-dropzone&from=2018-10-05&to=2024-10-05 vs. effort to do it right: https://github.com/KingSora/OverlayScrollbars/issues?q=is%3Aissue+is%3Aclosed. The platform exposes some primitives to customize this, e.g. https://codepen.io/oliviertassinari/pen/PoMzdJa Screen.Recording.2024-10-06.at.22.37.01.movbut until they fix w3c/csswg-drafts#9826, w3c/csswg-drafts#10591, it doesn't feel like a viable option, far from it.
Premium plan: no. Pro plan: maybe for the styled layer it would make sense? It might be pushing it too far to have this as a pro feature in Base UI. I get the feeling that 3 years from now, this component is no longer needed, built-in into the platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romgrk implemented a custom scroll container for the data grid https://github.com/mui/mui-x/blob/master/packages/x-data-grid/src/components/virtualization/GridVirtualScrollbar.tsx. Maybe this can be of any help. For example, can be seen in https://mui.com/x/react-data-grid/#pro-plan.
<div | ||
ref={tableWrapperRef} | ||
style={{ | ||
display: 'table', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Built-in inline style? Not sure this will fly with CSP.
*, | ||
*::before, | ||
*::after { | ||
box-sizing: border-box; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. In the visual regression, we do the opposite, so a great way to ensure that our style works with both models:
base-ui/test/regressions/TestViewer.js
Line 52 in d29192a
box-sizing: content-box; |
Edit: oh, wait, it's broken, fixed: #707.
0666187
to
a27b78e
Compare
The CSB version of the intro demo (https://codesandbox.io/p/sandbox/pydrhc?file=%2Fsrc%2Findex.tsx) looks slightly broken: Likely a box-sizing issue? As for the implementation, I couldn't spot any issues. Good work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you folow the patterns from #735?
(undefined
instead of null
and error message)
Same in ScrollAreaRootContext.
Took it for a spin. Many notes, but overall it's great. I love how much control I have over when exactly the scrollbar would be shown or hidden.
|
I think we can remove them by default, but Safari requires a
I got this naming from VS Code: it has a feature called "inlay hints" which are inline with the flow of the code
I think we can remove the scrollbar styles by default now, but removing the scrollbar is a lot easier than defining the padding (and the element node is not actually exposed by default).
There's a ResizeObserver but it might need to be lifted up to the root if it's defined on the viewport. Will check this.
I don't think this is possible to fix since it uses a
They're visible by default, there's no
|
Is this approach too heavy-handed? I asked specifically because I had run into the
Yeah Safari is the main problem. We don't have a precedent for rendering a
I'm seeing a I'd expect to either not have this element rendered at all and force mount it, or have to handle the visibility myself.
I think it's best when data attributes describe the state of the component itself rather than what the user is doing; in this sense |
There are two separate "hidden" cases:
I considered the option 1 to mean you never want to show it. The Due to transitions/animations, it makes sense to keep it mounted at all times as they're easier to create this way. Unlike floating components that can contain expensive content and create lots of elements in the DOM, scrollbars are cheap. For consistency with other components, if we adopt the
While this works for It looks like this also unfortunately breaks being able to create a "hit area" around the thumb as Radix does, in order to be able to click it more easily. Edit: I think preventing scroll of the parent at all times regardless of
I don't think we've used it yet, but I suppose Radix's approach will work. |
df1ba95
to
dde1b59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks incredible, can't wait to use it
a8e34ff
to
150792a
Compare
Closes #649
Preview: https://deploy-preview-665--base-ui.netlify.app/components/react-scroll-area/