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

Use custom scrollbar for GroupSelector #62

Merged

Conversation

JordanSjodinFaithlife
Copy link
Contributor

@dustinsoftware

The group selector has some complicated scrolling behavior, and not all browses displayed the scrollbar above the content.

Using css to attempt to alter the scrollbar was not a viable solution due to Firefox not supporting the css required.

Using a custom scrollbar allows us to have control over the location of the scrollbar and get a consistent look accross browsers.

Plus, fixed some small buttons on secondary modal content and fixed a gap in the header.

The group selector has some complicated scrolling behavior, and not all browses displayed the scrollbar above the content. Sometimes, placing the content above the scrollbar.

Using css to attempt to alter the scrollbar was not a viable solution due to Firefox not supporting the css required.

Using a custom scrollbar allows us to have control over the location of the scrollbar and get a consistent look accross browsers.
background: white;
border-radius: 4px;

.${ModalScrollViewContentClass} {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to pass in a reference to another component than to create a class name for it, can we do that here? Docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can get around using this class name

@@ -8167,6 +8197,14 @@ react-router@^3.2.0:
prop-types "^15.5.6"
warning "^3.0.0"

react-scrollbar@^0.5.4:
version "0.5.4"
resolved "https://registry.yarnpkg.com/react-scrollbar/-/react-scrollbar-0.5.4.tgz#270576a28d0f7783b2304b284dbd38d82b36b0e0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit worried that this dependency (and react-motion) are going to bloat the main bundle. If we need this dependency, let's move anything that uses it to a separate bundle. This is how we did it for text-input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try this

@dustinsoftware dustinsoftware merged commit e5ba146 into Faithlife:master Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants