Skip to content
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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Sep 30, 2024

@mui-bot
Copy link

mui-bot commented Sep 30, 2024

Netlify deploy preview

https://deploy-preview-665--base-ui.netlify.app/

Generated by 🚫 dangerJS against 38d38fb

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 30, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 30, 2024

It's about mui/mui-x#510, right? I have added the reference so developers can find a clear cross-link:

SCR-20241001-bikq

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.

@colmtuite
Copy link
Contributor

colmtuite commented Sep 30, 2024

@oliviertassinari

It's about mui/mui-x#510, right?

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.

most applications don't have a custom scrollbar

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.

We could envision this component under the Base UI X brand umbrella

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.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 6, 2024

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.

@colmtuite In any way, I mostly wanted to create a backlink between these two so it's easier to discover the link.

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.

For peak execution, I very much agree.
For the average UIs, I don't think it's a big need, it's common to not see it used in average UIs, e.g. I personally never implemented a custom scrollbar 😄.

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.mov

but until they fix w3c/csswg-drafts#9826, w3c/csswg-drafts#10591, it doesn't feel like a viable option, far from it.

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.

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.

Copy link
Member

@oliviertassinari oliviertassinari left a 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',
Copy link
Member

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.

Comment on lines +7 to +12
*,
*::before,
*::after {
box-sizing: border-box;
}

Copy link
Member

@oliviertassinari oliviertassinari Oct 6, 2024

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:

box-sizing: content-box;

Edit: oh, wait, it's broken, fixed: #707.

docs/next-env.d.ts Outdated Show resolved Hide resolved
@atomiks atomiks marked this pull request as ready for review October 8, 2024 01:16
@michaldudak
Copy link
Member

michaldudak commented Oct 11, 2024

The CSB version of the intro demo (https://codesandbox.io/p/sandbox/pydrhc?file=%2Fsrc%2Findex.tsx) looks slightly broken:
image

Likely a box-sizing issue?


As for the implementation, I couldn't spot any issues. Good work!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 14, 2024
Signed-off-by: atomiks <cc.glows@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[scrollarea] Implement ScrollArea
5 participants