Adding an id to the containing div

[Zloka]

Hi! First of all, thanks for the fork and revival.

My scrollable list component renders a div that I would like to replace with Scrollbars. It works fine out of the box, but it doesn't seem that Scrollbars supports an id prop, which I am currently passing to the div in order to lazy-load images using the intersection observer API.

Would it be possible to support an id prop?

[Tomassito]

There's a bunch of native HTMLElement div props that could be passed to the root element. In order not to pollute the TS hinting and decrease readability, I suggest to group these props under one Scrollbars props key. e.g. rootElementProps

[Zloka]

@Tomassito Sounds reasonable. I will look into making a PR for this if time allows for it this weekend.

There's a bunch of native HTMLElement div props that could be passed to the root element.

For my particular use-case the id is enough, but I suppose it is trivial to also support others. Any preferences on what I should narrow down the "bunch" to in that case?

Any preferences as far as testing goes? Again, I guess that providing e.g. an id and testing that the root element indeed gets this attribute is straightforward, though it might be cumbersome to do that for every native div prop we decide to support.

[Tomassito]

@Zloka I would allow any of the HTMLElement

properties. When it comes to testing the tricky part is that they don't seem to be configured to work with the dumi tool :/ https://github.com/sakhnyuk/rc-scrollbars/pull/11#issuecomment-774491341 But yeah, checking if all the passed props (constrained by TS type) get applied is the simple way to go. You don't need to check every prop, just a few.

[Zloka]

Opene a PR #24 for this just now.

@Tomassito @sakhnyuk any opinions? Like I mentioned in the PR, I wasn't able to run the tests locally so I didn't add any test cases just yet. They should be fairly trivial to add, but I opted not to just yet since I wouldn't be able to run them anyways.

[sakhnyuk]

Hi, @Zloka Sorry, for the late answer and thank you for your PR.

I'm gonna fix the tests in the next releases (was kindly busy the last few weeks).

For now, I just need to test manually your approach. And I think providing all native div props to our root element is a bad idea. Just for example:

• div has a native onScroll event and we can pass onScroll callback to the rootElementProps
• what about ref? Somebody will try to pass ref to there

What if we just add an id prop? This is the easiest way in my opinion.

PS: @Zloka Do you have time to wait for release? I can try to fix the test in few days. Or not, we can merge that feature ASAP

[Zloka]

@sakhnyuk Hi!

In my case, since I only actually need id I'm fine with that. If you want I can update the PR. Not really in a hurry, this isn't in production for us yet and the Scrollable still works as expected, it's just that the lazy-loading measurement is incorrect but we can live with that until this PR is merged! :)

[sakhnyuk]

@Zloka Yeah, it would be great if you'll update PR. So, after that, I'll merge PR and start working on tests.

[Tomassito]

@sakhnyuk This is why we'd group the 'divProps' under one prop so that there's a clear distinction. What if next person comes and says they want to provide x prop to that wrapper div? If they had a valid case, you'd have to extend CustomScrollbars by yet another prop?

[sakhnyuk]

@Tomassito I understand what are you talking about, but if we provide passing HTMLAttributes props, we must be sure that everything works fine. What if the next person opens an issue with the problem when he passing onScroll and ref and something else to divProps and it doesn't work properly.

Apart from the root, we can pass custom elements by render-props. What if we provide also the root component by render prop? Also, there we will able to control some inner root props (overwrite them for example)

[Tomassito]

Ok. Actually I can't come up with any other prop that users may want to pass.

On Thu, 25 Feb 2021, 16:36 Michael Sakhniuk, notifications@github.com wrote:

@Tomassito https://github.com/Tomassito I understand what are you talking about, but if we provide passing HTMLAttributes props, we must be sure that everything works fine. What if the next person opens an issue with the problem when he passing onScroll and ref and something else to divProps and it doesn't work properly.

Apart from the root, we can pass custom elements by render-props. What if we provide also the root component by render prop? Also, there we will able to control some inner root props (overwrite them for example)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sakhnyuk/rc-scrollbars/issues/22#issuecomment-785991594, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI6RXCRTNHMNMUAOJW7CRWDTAZU73ANCNFSM4XUUMLEQ .

[Zloka]

@sakhnyuk I updated the PR just now, didn't have time last evening I'm afraid. I added a test case which I believe should work, but feel free to alter/move it somewhere more appropriate if necessary.

For what it's worth, in principle I agree with @Tomassito that in the long run it would be clearer to support "all" props, rather than picking and choosing which to support and which not. If that need ever arises in practice is hard to say, and for my own purposes this will do.

[Zloka]

@sakhnyuk Hey, figured I'd give you a friendly ping in case you forgot! :)

[sakhnyuk]

@Zloka Thank you, for your remind 😁

I thought about a solution. Let's merge id prop without special divProp. Will see on the next proposal, what we could do with it. Today later, I'll check and merge that PR

[sakhnyuk]

I've merged PR.

Thank you guys @Zloka @Tomassito 👍