Skip to content

Conversation

@austingreendev
Copy link
Contributor

🚧 This PR doesn't have any testing yet 🚧

Description

This is an intro PR for the new react-scrollbars package. The API of react-custom-scrollbars is very restrictive, so there are some weird rendering flows and utility methods in this one.

Pre-published at: https://garden.zendesk.com/react-components/scrollbars for some 👀

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 💅 view component styling is based on a Garden CSS
    component
  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 💂‍♂️ includes new unit and snapshot tests
  • 📒 any new files are included in the packages src/index.js export
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

@coveralls
Copy link

coveralls commented Sep 19, 2018

Coverage Status

Coverage decreased (-1.6%) to 94.29% when pulling 26b39c1 on agreen/scrollbars into 702cc87 on master.

to view all of the available customization props and methods.

```jsx
const StyledSection = styled.div`
Copy link
Member

Choose a reason for hiding this comment

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

It would be preferable to use the MD Typography component here (and throughout).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.


```jsx
const StyledSection = styled.div`
margin-bottom: 16px;
Copy link
Member

Choose a reason for hiding this comment

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

Use the zdSpacing (20px) variable here (and throughout).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

{...this.removeInvalidStyling(props)}
className={classNames(
ScrollbarStyles['c-scrollbar__thumb'],
ScrollbarStyles['c-scrollbar__thumb--horizontal']
Copy link
Member

Choose a reason for hiding this comment

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

This is not a real class (incorrect CSS documentation). Style is determined by the orientation of the parent (.c-scrollbar__track--horizontal).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

{...this.removeInvalidStyling(props)}
className={classNames(
ScrollbarStyles['c-scrollbar__thumb'],
ScrollbarStyles['c-scrollbar__thumb--vertical']
Copy link
Member

Choose a reason for hiding this comment

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

This is not a real class (incorrect CSS documentation). Style is determined by the orientation of the parent (.c-scrollbar__track--vertical).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

data-garden-id={COMPONENT_ID}
data-garden-version={PACKAGE_VERSION}
className={classNames(ScrollbarStyles['c-scrollbar'], {
[ScrollbarStyles['c-scrollbar--dark']]: dark,
Copy link
Member

Choose a reason for hiding this comment

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

doesn't appear to be working in demo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be working. Can we do another walkthrough?

return (
<Scrollbars
hideTracksWhenNotNeeded
thumbMinSize={52}
Copy link
Member

Choose a reason for hiding this comment

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

Does the underlying component provide a reasonable thumbMinSize default?

{...this.removeInvalidStyling(props)}
className={classNames(
ScrollbarStyles['c-scrollbar__track'],
ScrollbarStyles['c-scrollbar__track--vertical'],
Copy link
Member

Choose a reason for hiding this comment

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

Garden's track styling doesn't appear to be coming through.

@austingreendev
Copy link
Contributor Author

@jzempel and @ryanseddon I just refactored this to move to the perfect-scrollbars library. This versions seems a lot cleaner and extensible than react-custom-scrollbars.

import classNames from 'classnames';
import styled from 'styled-components';
import PerfectScrollbar from 'perfect-scrollbar';
import { isRtl, retrieveTheme } from '@zendeskgarden/react-theming';
Copy link
Member

Choose a reason for hiding this comment

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

Horizontal scrolling for (not so)PerfectScrollbar is completely broken for RTL. Vertical isn't great, either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm seeing some weird behaviour with rtl mode on

@austingreendev
Copy link
Contributor Author

This PR has gotten stale. Lets close until it's time to revisit this experiment.

@jzempel jzempel deleted the agreen/scrollbars branch June 14, 2019 00:34
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.

5 participants