support className on root component;

[Tomassito]

fix prettier issue with line ending; sort props/definitions and class members

[vercel[bot]]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sakhnyuk/rc-scrollbars/2u4rf5pod
✅ Preview: https://rc-scrollbars-git-fork-tomassito-supportclassnameonroot-6cd248.sakhnyuk.vercel.app

[sakhnyuk]

@Tomassito Are you still working on a feature with classNames?

[Tomassito]

I've just come back from a holiday break. Guess we can have both props: className (for the root element) + classes {root: string, view: string, ...} (for all, with root repeated) here? You suggest I provide it under the same PR?

[Tomassito]

@sakhnyuk I've added the classes prop now. Please have a look. :)

[sakhnyuk]

Hey! Thanks for commits.

But what about classnames, that comes from renderView, renderThumb... props?

[sakhnyuk]

I mean cloneElement method overwrite props from source component. I think we should save fully capability with old API.

You need to think about merge classnames.

And one more think 😁: We are have to update Documentation with new classnames updates in that PR

[Tomassito]

Hello! Thanks for the input!

Please see the latest changes. I don't merge the classNames - if someone wants to take control over sub-element render then they are responsible for providing all they want there. I didn't update the docs/examples yet because wanted you to take a look at the approach first. Also, I get errors when trying to run tests:

cross-env NODE_ENV=test karma start

WebpackOptionsValidationError: Invalid configuration object. Webpack has been initialised using a configuration object that does not match the API schema.

• configuration.module has an unknown property 'loaders'. These properties are valid: object { defaultRules?, exprContextCritical?, exprContextRecursive?, exprContextRegExp?, exprContextRequest?, noParse?, rules?, strictExportPresence?, strictThisContextOnImports?, unknownContextCritical?, unknownContextRecursive? , unknownContextRegExp?, unknownContextRequest?, unsafeCache?, wrappedContextCritical?, wrappedContextRecursive?, wrappedContextRegExp? } -> Options affecting the normal modules (NormalModuleFactory).

[sakhnyuk]

I'll try to run your approach. Actually, you need to pull the last updates from master branch. After that, you can run yarn dev command and work with the demo page from documentations.

[vdjurdjevic]

@sakhnyuk What's the current state of this pull req? Is there anything I can do help speed this up?

[Tomassito]

I'm waiting for @sakhnyuk to give a 'green light' to these so that it makes sense to update the docs/tests.

[sakhnyuk]

Hey, @Tomassito

Are you meaning that you've done with classNames feature?

I've cloned your branch and I no see scroll thumbs there.

[sakhnyuk]

@Tomassito

Could you pull master branch to that branch (and PR)?

[Tomassito]

@sakhnyuk Sure, done! I'll add a change regarding https://github.com/sakhnyuk/rc-scrollbars/issues/13 in a sec.

[vdjurdjevic]

@Tomassito I have one more suggestion for this feature. What do you think of adding default classes to all parts of the scrollbar, and merging them with externally provided ones? For example, you could add .rc-track, .rc-handle, ... etc. These classes would be used to target child elements from the root element. One use case is using styled-components. I can only style root component with styled-components, and I need a way to target these child elements. Code example:

import styled from 'styled-components';

const CustomScrollbars = styled(Scrollbars)({
    '& > .rc-handle': {
        background: 'red'
    }
});

[sakhnyuk]

Alright, I've added some fixes and now we have refactored scroll elements.

Actually, I see a few problems with backward compatibility and classNames.

Problem 1

We need to support 2 ways to set classNames to components.

class CustomScrollbars extends Component {
  render() {
    return (
      <Scrollbars
        renderView={props => <div {...props} className="FIRST"/>}>
        classes={{ view: 'SECOND' }}
        {this.props.children}
      </Scrollbars>
    );
  }
}

As a result, we should see className="FIRST SECOND"

And maybe default className suggested by @vdjurdjevic.

Problem 2

How is the user will able to style tracks and thumbs when default styles providing from inline styles?

I mean, height/width/backgroundColor will be overwritten from style prop.

What do you think guys?

[sakhnyuk]

@Tomassito you can pull the last changes and use "yarn dev" command to start the project locally.

[vdjurdjevic]

@sakhnyuk For problem 1, I would merge all of the classes (there are utilities like classNames and clsx). Additionally, you might provide the option mergeClassNames: boolean to let the user disable merging. For problem 2, maybe provide option useDefaultStyles: boolean. If user passes false, he is responsible for styling it with css. If you calculate height dynamically with javascript, this should be always set as inline style. Only let user choose to provide custom styles for stuff like color, width, etc.

[Tomassito]

Hi gents, Please take a look at the latest draft changes I pushed. It is just to show the approach, not final changes.
Examplary changes are on the 'container' aka 'root' and 'vertical thumb. The colored example provides 'test' className just to prove that it works as expected. Tentative classNames for each subComponent are provided + calculated some of the dynamic ones (root). Still had to provide {} empty object to the renderer function, in case someone was drawing some values from the style object. Also we don't get rid of all the inline styles (for the dynamically calculated values) but can't see how we could do this without adding some more dependencies (which, I don't think we want).

Regarding the problems mentioned above:

1. I wouldn't worry about this one. When you use the render function you are responsible for the providing the props and have the power to shape them the way you want. Power = responsibility. Simple. Take example of 'children' prop in React : should they merge them if provided via props object and via JSX at the same time? No, one overrides the other.
2. If we extract to classNames all the 'visual' styles while the 'functional' ones stay as dynamic ones in styles - I see no major issue here. If someone really wanted to do it anyway then : a) they can use render function b) they can resort to '!important' in css

[vdjurdjevic]

@sakhnyuk @Tomassito

The main feature is to enable easier customization without custom elements. For people using styled-components or tailwindcss, it's a lot faster to just use className, and classes API. Think of use cases where we just want to change the color or width of a scrollbar in different places in the application. So merging is the only path that makes sense. If the developer provides a custom element, he is responsible for everything. So I would definitely merge classes. Now, instead of installing classes utility, we can implement it ourselves, it's trivial. It's better to keep this package as lean as possible. For css, dynamic values should be added with inline styles (current behavior), but pure styling values like colors, border-radius, width (height for vertical) only if the developer does not disable it. So we could provide the option useDefaultStyles: boolean which is true by default. If the developer passes false, we just don't set these styles and let the developer do that with className and classes API.

What do you think?

[sakhnyuk]

If I got you right, you propose the same things, as I said above with some additions.

Basically, we decided to stay on inline styles, but with a prop for disabling it?

So, repeating my points:

• Providing default classNames
  • for styled-components and tailwind
• Render props + inline styles + className (what you want)
  • for backward compatibility
  • for those who want to pass their own component
• classes props (with merging all classNames)
  • for setting classNames easy
• Comprehensive documentation about how to customize your scrollbars
  • Use !important or set useDefaultStyles=false
  • Demo with all types of customization

@vdjurdjevic is it right? @Tomassito what do you think?

[vdjurdjevic]

Exactly!

I will just write one more time how I see classes API, just in case.

• Root element has className property that user can pass. In addition to that, we merge marker class name to that for example .rc-scrollbars-root
• Root element also has classes property that user can use to pass className to inner elements. Same logic applies, we merge marker class name with that, for example .rc-scrollbars-horizontal-handle

So, for example tailwind users can set classes for inner elements easy, styled components users can target inner elements from the root with marker classes.

[Tomassito]

Providing default classNames for styled-components and tailwind

I'd say to promote avoidance of inline styles. The styling tool doesn't matter much.

Render props + inline styles + className (what you want) for backward compatibility

Ok. So the default classNames would only be 'markers' without any styling then.

for those who want to pass their own component

Do we agree that there's no point in trying to merge classes provided in render function with the 'classes' prop of the Scrollbars?

classes props (with merging all classNames) for setting classNames easy

By 'all' we mean 2: the 'marker' class + provided (if any), right?

Comprehensive documentation about how to customize your scrollbars Use !important or set useDefaultStyles=false

I don't see much usage of 'useDefaultStyles' prop - did you see how little styling there actually is? Bacground color & border radius... Is this going to save devs time in customizing looks? Not too convinced on this one.

[vdjurdjevic]

I'd say to promote avoidance of inline styles. The styling tool doesn't matter much.

Yeah, but with (className + classes + marker classes) we made sure to make it easy to customize for people using different styling approaches (I use both on different projects)

Ok. So the default classNames would only be 'markers' without any styling then.

Correct!

Do we agree that there's no point in trying to merge classes provided in render function with the 'classes' prop of the Scrollbars?

Yep! If the user wants full control, let it be.

By 'all' we mean 2: the 'marker' class + provided (if any), right?

Correct!

I don't see much usage of 'useDefaultStyles' prop - did you see how little styling there actually is? Bacground color & border radius... Is this going to save devs time in customizing looks? Not too convinced on this one.

We need this to avoid using !important. While it's only two css properties, important is a last resort solution and should be avoided whenever possible.

[Tomassito]

Ok, so I reverted the changes in the wrong direction ;) And introduced the ones agreed. If you have any comments please provide. (The ones with code references like @sakhnyuk has made above are great). If all is as expected, I'll follow up with the documentation. PS. Vercel doesn't want to share anything about failure reasons. Would be hand to have this a bit more open.

[sakhnyuk]

@Tomassito Before push, you can try to build the project. "yarn prepublish" "yarn build:docs:

[sakhnyuk]

I've found one little bug:

undefined in classNames. Just need to add null-check and don't pass className when prop is undefined.

So, everything else works well 👍

Also, I'll check the code, maybe will add some fixes, or comments.

[Tomassito]

I've added a touch of documentation and wanted to add a test or two but Karma doesn't even want to start for me and complains about webpack configuration. Are you able to run the test without problems?

[vdjurdjevic]

When can we expect this to be merged? I need this for a new project asap.

[sakhnyuk]

When can we expect this to be merged? I need this for a new project asap.

I have checked the code and added some fixes. Today I am going to push it. Also today or tomorrow I will merge the PR.

[sakhnyuk]

I've done what I planned before merge - update the customization page and add some fixes.

here is a preview doc - https://rc-scrollbars-l30ovxix8.vercel.app/

[sakhnyuk]

@Tomassito about tests with karma. In my opinion, this should be in the next PR. Cause we need to rejuvenate the test environment with some old tests.

[vdjurdjevic]

@sakhnyuk @Tomassito Awesome work! Thanks! :)