Closed KingSora closed 1 year ago
Hi @KingSora , Good day
i suggest that you keep browser support but in a modular way or maybe have a separate version/release to support it but if all future features isn't feasible in older IEs and if current version 1 of library is stable or will be eventually in the future then dropping IE support is not that bad
Keep up the great work! πͺπ
Hi. Please consider supporting css-modules. The desired API could look like this:
<Scrollbar
options={{
<cssModules?>: true/[false], //if true, import theme stylesheet and use generated class-names, else use unscoped class-names
<theme?>: ['dark']/'other themes...', //defines which built-in theme to require
<classMap?=null>: {
host: 'additionalClassName',
//...
viewport: 'additionalClassName'
}
}}
>
What is the gist of this:
!important
, if user want's to increase the specificity or overwrite the defaults, he/she can achieve this via classMap
property (useful not only for css-modules)If you need any clarification or have any concerns, let's discuss them
Hey @KingSora, This is a great project. Huge thanks to you for your efforts! All the changes you have in mind sound good to me. The main expectations I have on a JS scrollbar library is that it get's out of your way, is performant, integrates with frameworks and does a best effort to support popular virtual scroll implementations. This project checks all those boxes and any improvements on these fronts would be impactful imo.
I'm considering OverlayScrollbars as a solution to a long standing problems we've had with Chromium scrollbars on Spotify. The most important features for us are performance and control over styling.
@tryggvigy cool! I'll give my best to achieve the goals vor v2.0.0
. I hope OverlayScrollbars
fits your needs and will perform well, as my focus was exactly on performance, compatibility and styling in the current v1.x
version.
For React version, I would like to access ref to scrollable div, i.e. <div class="os-viewport">
to be able to save position of the scroll throughout browser history changes. This would be useful when the OverlayScrollbars are used for main div, but url is switching with ReactRouter.
I know that there is an option to access this element by instance.getElements("viewport");
but this is not React Ref Object. The React-way should return Ref Object, not the exact DOM element.
It's fairly easy to do this today
<OverlayScrollbarsComponent
ref={ref => {
myScrollNodeRef.current = ref.osInstance().getElements().viewport;
}}
>
Thank you for your answer @tryggvigy, I do it in similar way now:
const osInstance = scrollbarsRef.current?.osInstance()?.getElements("viewport");
const scrollbarsViewportRef = osViewport ? { current: osViewport } : null;
...
<OverlayScrollbarsComponent ref={scrollbarsRef}
but it looks like a workaround not the long-term react-way solution. I believe that API of any react component which renders any DOM node should have an ability to pass a ref of that node to an app. Unfortunately React team does not provide recommendations for developers how to create components to make them compatible to each other.
@seleckis there are some issues I have with designing such a API.
I could simply use a forwardedRef
to the viewport element, this would solve your particular case. But with this approach you can't get the osInstance
anymore, because you need the ref
/ HTMLElement
to the host element and not to the viewport element to get it. And since you can get every DOM element you need when you have the osInstance
this would be more of a downgrade to me.
In react you have RefObjects
and these objects have a current
property which contains the corresponding HTMLElement
. Does it really make so much difference to you when you get the viewport HTMLElement
from the osInstance
instead of the RefObject
?
I'm aware that it would be "more beautiful" when the lib would behave like a "true" react lib, but since this isn't the case and I want to have the wrapper components
as simple as possible for now (because of easier maintenance). This can change in the future obviously, but for now I have no plans to change the wrapper components
since you can do everything with them once you have the osInstance
.
@KingSora I see, alright, thank you
Hi @KingSora,
i really like OS, it's a great tool. I am using it with anular/kendo library and i did not find a way to add it in popup/modals(dropdown/combobox).
can you tell me please if it works ?
And also wich classnames i can add manually to make it work inside popups modals.
Thanks.
I added it with, but every time i close/open popup is gone(it's not part of DOM)
OverlayScrollbars(document.querySelector(".k-list-scroller"), this.osComponentOptions);
Thanks a lot!
Dropping support for IE8-10 seem absolutely natural. Nowadays the minimum reasonable expectation is IE11 (and only for government office whose computers, etc). And even IE11 is questionable (see: IE11 Mainstream End Of Life in Oct 2020 https://www.swyx.io/writing/ie11-eol )
A good compromise would be adopt a more plugin/extension architecture (optional support for the scroll method, IE11, etc), so that the core library is more focused and easier to maintain.
Hi @KingSora Overlayscrollbar is a great library. I used many such libraries before and they have a lot of unexpected issues.
Now I see that your library has angular 7 in peer dependencies. It looks like it works OK with Angular 9, however it would be better to update overlayscrollbars-ngx to use angular 9. At least in 2.0 version. What do you think?
I can try to help you with it :)
Also, I have some cool improvement for overlayscrollbars-ngx
. It's about configuration. I almost done it in fork repository, but I have some questions. I'm going to add issue and describe them.
@NechiK feel free to create a corresponding issue here in github where we can discuss such things :)
Short update for v2.0.0
development: I've decided to use TypeScript
for the entire project and modularize it (as currently in v1.x
everything is in one big file).
Thanks to the IE drop, I could get rid of many lines of fallback and compatibility code. I'll also get rid of the "jQuery" like coding style since it brings a lot of issues in terms of event listeners etc.
Would it be out of scope to consider just adding virtualization capabilities into an optional dependency, rather than trying to support any virtual scroller?
I feel like it might be more effort to support virtual scrolling libraries, than just rolling your own.
@Nikey646 thanks for you suggestion! I've thought about that too, but in the end I came to the conclusion, that support for virtual-scrolling libraries can't be added as a plugin, because its too much interweaved with the initialization process.
I'll realize this support in a general way. Please don't think about it as a solution per library (the code-base won't grow with every supported virtual-scroller). The solution I'm aiming for is a initialization process, which is so flexible, it can work nicely with the structure most (if not all) virtual-scroller libraries have. This initialization process is a bit more complex than the "normal" one, but it is optional and for "more experienced users". With this new optional system, there is also support for almost any kind of library (dropdowns, grids etc.) because most of them follow a core pattern structure (one host element, and one viewport element).
I'll also definitely give my best to design this system in the most lightweight way possible.
I would like to suggest one option for better styling and customization. Material-UI for React has a very powerful API for complex component customization. They provide classes object, that one can use to assign a custom class to nested elements. The same approach could be used to customize parts of the scrollbar or host element. Here is more information: https://material-ui.com/customization/components/
Also, for react wrapper, it would be great to support css in js approach for base styles instead of loading global external stylesheet. I could help with that if you accept contributions. It's basically just converting existing stylesheet into css in js equivalent, and leaving option for providing css-in-js solution to be used to that (so that you don't couple with any of them).
@vdjurdjevic Thanks for you suggestion! I'm familiar with CSS-in-JS and in my oppinion it doesn't really have any benefits compared to (S)CSS modules (which are basically normal style sheets), but this is just a personal opinion.
The plugin right now offers almost infinite possibilities of style customization, you just have to write your own theme which is documented here.
OverlayScrollbars will always follow the approach that JS (logic) and CSS (styling) are two things which won't be mixed. That beeing said, the core library will also be always dependent on its stylesheet, because it is the most straight forward way to implement it, without adding additional complexity. The workflow should be:
This is something everyone can do, no matter which project structure is used. In terms of customization I still have to decide whether I'll implement someting like @Media-Evil suggested.
Well, I don't know what kind of benefits you compare with sass, but css in js is not just about how you write your css, it's about better dynamic styles, minimal css in production, more confidence when refactoring, modifying, deleting parts of css, automatic vendor prefixing, etc.
But anyway, I am not talking about core lib, just react wrapper. Core lib should stay the way it is, compatibility with any environment. What @Media-Evil suggested is similar to what I suggested with classes property. It could be added to core lib since it's just another option to set different parts of class names. I will try to explain better why this is needed.
As you said, "The plugin right now offers almost infinite possibilities of style customization, you just have to write your own theme which is documented here". And you are right, it can be customized, but you have to write a global theme with complex selectors to target parts you want, and it's impossible to do without checking documentation every time. For example, you have to target .os-scrollbar-vertical > .os-scrollbar-track > .os-scrollbar-handle
to customize handle. The idea is to provide classes object in options that looks like something like this:
export type ScrollStyles = Partial<{
root: string;
horizontal: string;
vertical: string;
track: string;
horizontalTrack: string;
verticalTrack: string;
handle: string;
horizontalHandle: string;
verticalHandle: string;
}>;
You can use this object to optionally assign additional classes to parts of the element tree, and this can be part of generic core packing, it's not coupled to any css technology.
This way we could at least generate this customization classes with css in js and change them dynamically, and load default css with an external stylesheet.
@vdjurdjevic ouh, my bad! I interpreted your first proposal as if you want replace the stylesheet completely with CSS in JS stuff. Alright then, after you've cleared things up now, I'll tell you my thoughts on how I planned to implement this:
The one thing which was bothering me the most was that this object would be just really big and verbose and would enlarge the already large options object. This was, because I thought this object should have default values:
{
host: 'os-host',
scrollbarHorizontal: {
scrollbar: 'os-scrollbar-horizontal',
track: 'os-scrollbar-horizontal-track',
handle: 'os-scrollbar-horizontal-track-handle'
}
/* ... */
}
But then I thought again about it, and what if the default value for everything is just null
and the styles provided by you, must simply override the "real" default styles which are always there? With this approach I could make usage of the extension system I've build, and shift this entire functionality into a extension
. Then this extension takes care about adding and removing class names according to the provided extension-options.
I think this would benefit everyone, since users which don't need that functionality don't get that code shipped per default and users which need it, simply can use the extension inside their project. The plugins options object isn't touched since each extension can have its own defined options. Also this can then be used with the core library and with ever wrapper, because every wrapper supports the extension system already.
In the end code would look something like that:
// vanilla initialization
OverlayScrollbars(/* element */, { /* options */ }, {
"cssInJsExtension": {
host: 'os-host',
scrollbarHorizontal: {
scrollbar: 'os-scrollbar-horizontal',
track: 'os-scrollbar-horizontal-track',
handle: 'os-scrollbar-horizontal-track-handle'
}
/* ... */
}
});
// react
<OverlayScrollbarsComponent
options={{ /* options */ }}
extensions={{
"cssInJsExtension": {
host: 'os-host',
scrollbarHorizontal: {
scrollbar: 'os-scrollbar-horizontal',
track: 'os-scrollbar-horizontal-track',
handle: 'os-scrollbar-horizontal-track-handle'
}
/* ... */
}
}}
>
</OverlayScrollbarsComponent>
Obviously the strings would come from your CSS module or CSS in JS lib.
What do you think about this?
That's exactly what I am talking about :) Everything is optional, that's why I used Partial<{}> for type example. That way, it does not add an extra burden for users that just use defaults, and for those that want to customize, only handle, for example, it provides an easy and straight forward way, that can be done fast without checking theming documentation. And since these class names are just strings, it does not matter if one writes them in sass, on in style tag in index.html, or use Emotion CSS function to generate it at runtime. That's the approach that MaterialUI uses and it's extremely powerful in my opinion. It's the first material implementation that I stumbled upon that allows me to fully customize every component and fit it to my product design and avoid building Gmail clone :D
Now, I think that this is more option than extension semantically, but I don't mind if you make it as you proposed. My only concern is the ability to change it at runtime. Can you update extension settings at runtime?
Can we do something like this with extension settings?
scrollInstance.options('className', 'some-custom-class-name');
This is needed for dynamic styling. For example, I have 3 variants of the scrollbar in my application. I pick one of them based on the property. Or if I want to adjust it when the user switches from light to dark theme. Or provide user with the ability to tweak colors, sizes, etc.
Now for the react world, my proposal was to also add as an option not to include and bundle css file and load it as an external stylesheet but to provide it as css in js. For example, default option would be current one, where you include .css file and use OverlayScrollbarsComponent. Now, we could create another repository, styled-overlayscrollbars that uses styled-components and bundles all styles with react component instead of being dependant on external stylesheet for default styling. That way users that already use styled-components just need to install that package and use OverlayScrollbarsComponent without importing external stylesheet. It saves a network request and makes it more react way, component approach, where a component is isolated and does not rely on global stuff.
@vdjurdjevic The reason why I choosed the extension approach over the options approach is because even if the options are optional, their implementation inside the code-base is required. So users which won't ever use this functionality still have the code shipped because other users might do. Thats also the reason why I'll move the scroll
method into a extension. You don't have to, and might never use it, but it is still in the core code-base at the moment.
Now, I think that this is more option than extension semantically
You're right, but technically all extension-options are a way to extend the options object in a way where the required code is shipped separated from the code-base so users can decide whether to use it or not.
Can you update extension settings at runtime?
This is something every extension author has to implement by himself. I'll elaborate a concept for this in the future, so that it isn't as laborious as it would be now. But the general answer is yes if its implemented.
Ok, I am ok with that approach. As long as API like that is available, I don't mind how it's implemented.
Just small detail, you named it cssInJsExtension in example. It should be something like 'customStyles' or something like that. It's generic class name API, not css in js.
My opinion is that it should be part of the core, as an option, since I think that customization is more often used than not, at least people need to adjust colors. And also, it's a few if statements to apply those classes if provided, code it won't affect bundle size as much. When do you think some preview version of v2 could be available? I would like to start experimenting with it as soon as possible. I am about to start some internal project next month, where I can play with unstable stuff. I see that @tooppaaa has submitted a pull request for function based implementation of react component, so I could join forces with him to implement react components (both default and textarea), and also css in js version for the most popular css in js solutions.
@vdjurdjevic The extension name was just a name I came up with, it definitely has no relation to the real name of the extension.
My opinion is that it should be part of the core, as an option, since I think that customization is more often used than not
Yes, but you also can customize it directly with "global" styles, without any dependencies on css modules or anything like that. In this regards I really wanna stay as basic as possible and offer more possibilities as extension (thats why I made this system in the first place).
I'm not really sure when v2.0.0
will be finished, but I'll create a branch for it in the near future, so you can build and test it if you want. One thing: don't expect much change in the beginning because I'm currently rewriting and restructuring it in TypeScript which also takes time. And only after the whole rewritten thing passes all tests, only then I'll start implementing the new features stated in the first post. In this way I can guarantee to start with a base which has at least the same quality as the current version.
Great, looking forward to it :) I have one more question. Since you are dropping support for older browsers, will css file get smaller? I am thinking of making my own react wrapper with v1 until v2 is ready. I would move all styles in js. Current one is pretty big, i am just wondering if something can be omitted.
I made wrapper for material ui users with v1. You can check it out if you want :) https://www.npmjs.com/package/@vdjurdjevic/material-scrollbars
Branch vor v2.0.0
is now here: https://github.com/KingSora/OverlayScrollbars/tree/v2.0.0 for everyone whos interested.
Took a look. Pretty nice infra foundation laying.
Unrelated, but I ended up picking OverlayScrollbars to solve our problems and they are now in use on open.spotify.com! Thanks!
We did see a statistically significant drop in framerate but it was acceptable. The added JS work on the main thread is not much (although it's expensive when considered that 16ms is all you have each frame fro 60fps scroll. We have a few scroll-locked animations that are expensive and we certainly drop more frames with the overlay scrollbars in combination with those animations) but it's easier to cause unnecessary browser repaints. It was necessary to force os-viewport
to become a layer, otherwise scrolling framerate really suffered due to constant repaints while scrolling.
.#{$theme-name} .os-viewport {
transform: translateZ(0);
}
I benchmark our scrolling framerate using https://gist.github.com/bvaughn/1fc166d5cb3f8c174a1552eeaeeaa0e6 and then compare the results with a t-test:
Where the 95% confidence interval of the mean change (
95% CI
and 95% CI (%)
) is the most useful metric.
If you want to consider any of the performance or benchmarking aspects for 2.0 or the future I'm happy to talk more about that.
Overall, I'm very satisfied with OverlayScrollbars. Thanks!
@tryggvigy thank you very much for that insight! It would be very helpful for me, if we could talk more about that, so that I can improve in those areas even more in v2.0.0
. If it is okay and I'm in this step in development, I would come back to you for that topic.
Currently the plugin is doing most of the work inside the update code, and during scrolling I'm trying my best to use the cache produced by it. In the future I plan to decrease the javascript which is running during scrolling to 0
(or almost 0
). I plan to do this with the new Houdini Animation Worklet
in specifc with the ScrollTimeline. But since this API isn't quite ready yet, its still just a planned feature.
I'm really proud and happy that OverlayScrollbars
matches your and seemingly spotifys expectations, and I'll do my best to ship an even better version with v2.0.0
.
sounds great! Thank you for your time and contributions to the web ecosystem.
I just started using this package, I can only suggest making it lighter in term of size, maybe by dropping IE support or/and by split builds based on needed features like frameworks already do.
Also..would be great to be able to use the Vue package by directive too and not only by component.
Keep up the good work.
Add functionality of https://github.com/willmcpo/body-scroll-lock
@Alex-Sokolov you will be able to use this library with OverlayScrollbars v2.0.0
.
Small update here, since its been a while. Development of v2
is going well, I finished already a significant amount of work:
target
, padding
, viewport
, content
) is also done, incl. initialization choices for compatibility with other libraries (like Grid or List virtualization
libs). (code & tests)I'm using a new strategy in achieving the same scrollbar hiding functionality as in v1
but with significantly less code and improved performance. This wasn't possible before because I'm using flexbox
for this and since I supported IE8 in v1
I couldn't use it.
Also good news for modern browsers, since not all elements are necessary anymore the plugin will detect the minimal possible DOM for the current browser (this is also configurable to get the same DOM as in v1
even in modern browsers). The most minimal setup will just create the viewport
element and skip the padding
and content
elements.
Currently the Minified size is 20KB
, but I didn't do any optimization work yet.
There is still a bunch of stuff missing, but I'm working hard on it to finish all of this this year.
I case someone is interested in the code, you can check it here: https://github.com/KingSora/OverlayScrollbars/tree/v2.0.0
Thank you for maintaining this great library @KingSora!
Hi @KingSora I used your great library quite extensively in the past year, and about the only thing I think it's lacking at the moment - is the ability to separate user-generated scrolling from scrolling by calling scroll(). Currently either one generates a full set of events, callback calls, etc - virtually indistinguishable from each other in the code. This poses a problem in cases when you need to do extensive custom handling of user scrolling - typically, you want to handle user scroll events, but skip the scroll events that you caused with your own code. My current workaround is based on temporary switches (e.g. turn off user scroll handling, do scroll(), turn on handling again), but that's extremely inelegant.
Good day @just-dont
Well, the problem here is that the onScroll
callback is just a proxy for the native scroll event. The scroll offset can change through very different mechanisms like:
scroll
function (which only sets the scrollLeft and scrollTop properties to a calculated value)scrollTop
and scrollLeft
values directly by yourselfSo I really don't want to bring more complexity in by introducing a artificial boolean value which indicates whether you used the scroll
method or not, because it would increase the library size with a feature which would be probably used just by a minority. Also the value wouldn't be always 100% accurate because its possible to setup cases where you can't reliably detect whether the user or your code is scrolling for you. (Imagine you're a mobile user and youre trying to interfere with a scroll animation for example etc.)
I'm sorry but its very likely that this feature won't end up in v2
..
Why do I see that the v2
branch is not developed with typescript
?
@usercao what do you mean, its basically 100% typescript π
@usercao what do you mean, its basically 100% typescript π
Sorry, I read it wrong. I didn't click in to see the core part.
Good day! @KingSora Thank you for maintaining this great library! I wonder if there is any document about v2.0? I'm using v1.13 in a new project which will build soon and wanna replace it with v2.0. Thanks again!
Curious if there has been any progress on the v2.0.0?
After working almost non-stop on the plugin for the past month, I am happy to announce that I'm confident to share a at least a beta release at the end of the month.
That's awesome! Happy to help test it @KingSora
@tryggvigy thanks a lot! I'm really looking forward to your feedback :)
Thank you very much for your work! Is the beta already usable and could you please provide a simple example on how I can initialize it?
@AgentSmith0 its already usable but not released yet, because I'm still writing the README.md
for the most basic documentation. I'll announce it soon
Alright, thank you!
I've published v2.0.0-beta.0
. There is a basic README.md
but its not a complete documentation. In case you have questions please don't hesitate to ask!
Its almost feature complete, in case you discover something which doesn't seem to be supported but you think its crucial for the plugin, please also don't hesitate to write it here.
Expecting feedback of any kind! πΊ
Thank you! Is it possible to scroll while having the cursor over the scrollbar handle? (https://github.com/KingSora/OverlayScrollbars/issues/128, https://github.com/KingSora/OverlayScrollbars/issues/322)
EDIT: fixed
@AgentSmith0 nope.. I've noted it for the next beta (should be easy to implement)
Good day!
In this issue I want to start a discussion what you'd expect from
v2.0.0
of this plugin and what my plans are. Please feel free to post any suggestions or wishes you have, I'm open for everything! Also if you think any of my ideas are bad or aren't making much sense, this is the right place to adress it.Alright, lets start! My plans for
v2.0.0
:Browser support
IE8
, ideally I would drop alsoIE9 and IE10
soIE 11
would be the "lowest standard". This change would make the library smaller since less compatibility code is required.Breaking Changes
Currently I don't like how the instances work. Destroyed instances are basically useless. This shall change in
v2.0.0
as instances will be always of use, even if you decide todestroy
it, it will only destroy the generatedDOM
,EventListeners
,Observers
and so on. The instance itself can still be managed and you can also still utilize thescroll
function, so you don't always have to do a awkward check which scroll function (native or plugin) you should use.Instance management, this change can only be done if we support
IE 11
as the lowest standard. Instad of applying a new property to a DOM element aWeakMap
should be used for instance management.The scroll function. Currently the scroll function is relatively big and I don't really know how many users really needs it, so this change should definitely be discussed! The plan is to make it
optional
as astatic
method. So the standard lib comes without it but, you can always decide to download the extensions to use it. The usage would look something like this://get scroll info OverlayScrollbars.scroll(osInstance)
Features
Scrollbars management: Currently the scrollbars are applied inside the
host
element. Since this request was posted a few times in the past, it could be possible to pass a DOM element to which the scrollbars should be applied. (So scrollbars could be applied also outside of the generated DOM) This feature is also related to the Compatibility with other libraries feature since the DOM structure of such libraries sometimes requires a different parent element for the scrollbars.Compatibility with other libraries. Yeah, yeah, currently its hard to use
OverlayScrollbars
with other libraries for things likevirtualization
orGrids
, thats because often these libraries weren't designed with optional custom scrollbars in mind. I don't know exactly how it will turn out, but I think its possible thatOverlayScrollbars
can offer the possibility that at least aviewport
element can be passed somehow and then The library can adapt to that. The complex part is, that any library is designed differently and some libraries offer more elements than just aviewport
and some don't. So I would also add the possibility to pass following elements:host
,viewport
andcontent
, whereviewport
is required and the others are optional.Thats it for now! Thanks for your attention and please submit your ideas or any feedback you have.
@Grsmto @MarcosRakesh @Windvis @hamed-ehtesham @parsisolution @Acccent
55