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

Styling and appearance improvements #45

Closed
wants to merge 3 commits into from
Closed

Styling and appearance improvements #45

wants to merge 3 commits into from

Conversation

orangecoloured
Copy link

@orangecoloured orangecoloured commented Jun 26, 2018

Added customisable css class prefix.
Removed inner thumb.
Added scrollbar track custom class.
Added disappearing scrollbar toggle.

Now if a user uses a custom prefix everything's going to break, because the prefix in the css is hardcoded. It can be fixed by introducing inline styling for everything with ability to add user style objects. But at least now styles have their own namespace.

Also, heavily edited formatting, because eslinter wouldn't allow me to build.

…m class. Added disappearing scrollbar toggle.
@rommguy
Copy link
Owner

rommguy commented Jun 29, 2018

Hi @orangecoloured ,
I started to review the changes, why did you change all the spaces and added semicolons ?
Can you please revert those changes?
It makes it very hard to understand what changed from the files diff

position: absolute;
height: 100%;
width: 6px;
width: 6x;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are missing 'p', should be 6px

@orangecoloured
Copy link
Author

@rommguy
All the syntax changes were necessary so the linting would stop throwing errors. I couldn't make a build without it, so I had to make those changes.
As for the prefix and styling. As I mentioned above. One way to fix it is to use inline styling, so all the styles would stay in the context of the component.

@sag1v
Copy link
Contributor

sag1v commented Aug 14, 2018

any progress with this PR? something i can help with?

@sag1v
Copy link
Contributor

sag1v commented Aug 14, 2018

I think we could (or should) go with css modules for this use case.
Add this to the css-loader:
css-loader?sourceMap&modules&camelCase=dashes&localIdentName=rcs-[local]

And in javascript use it as follows:

//...
import styles from './cs.scss'
//....
return (
    <div className={`${styles.customScroll} ${this.state.onDrag ? styles.scrollHandleDragged : ''}`}
      //...

This will render to the DOM:

<div class="rcs-custom-scroll">
// OR
<div class="rcs-custom-scroll rcs-scroll-handle-dragged">

Of course for every change we made we need to update the docs, as this is a breaking change.

I do think this change is a must. as mentioned by @orangecoloured, the CSS classes that are used in this library are very common and may conflict with other global CSS classes.

@sag1v
Copy link
Contributor

sag1v commented Aug 18, 2018

I think we can close this as #51 solved it

@rommguy rommguy closed this Aug 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants