Merge pull request #73 from open-amdocs/scrollable-performance-refactor

open-amdocs · Nov 9, 2021 · adc99ad · adc99ad
2 parents c65e5c8 + 6144dd1
commit adc99ad
Show file tree

Hide file tree

Showing 12 changed files with 203 additions and 157 deletions.
diff --git a/src/components/Scrollable/Scrollable.constants.js b/src/components/Scrollable/Scrollable.constants.js
@@ -14,4 +14,11 @@
  * limitations under the License.
  */
 
-export const SCROLLING_CLASS_REMOVAL_DELAY = 500; // In milliseconds
+export const SCROLLING_CLASS_REMOVAL_DELAY = 500; // In milliseconds
+
+export const CSS_VARS = {
+    verticalRatio: '--scrollable-vertical-ratio',
+    horizontalRatio: '--scrollable-horizontal-ratio',
+    scrollTop: '--scrollable-scroll-top',
+    scrollLeft: '--scrollable-scroll-left',
+};
diff --git a/src/components/Scrollable/Scrollable.jsx b/src/components/Scrollable/Scrollable.jsx
@@ -23,7 +23,7 @@ import ResizeObserver from 'tools/ResizeObserver';
 import {VerticalScrollbarPlaceholder, HorizontalScrollbarPlaceholder, VerticalScrollbar, HorizontalScrollbar} from './components';
 import Context from './Scrollable.context';
 import {propTypes, defaultProps} from './Scrollable.props';
-import {SCROLLING_CLASS_REMOVAL_DELAY} from './Scrollable.constants';
+import {SCROLLING_CLASS_REMOVAL_DELAY, CSS_VARS} from './Scrollable.constants';
 import './Scrollable.scss';
 
 export default class Scrollable extends React.PureComponent {
@@ -33,10 +33,13 @@ export default class Scrollable extends React.PureComponent {
     constructor(props) {
         super(props);
         this.container = React.createRef();
-        this.vTrack = React.createRef();
-        this.hTrack = React.createRef();
-        this.ctx = {container: this.container};
         this.event = {prev: {}, next: {}};
+        this.state = {
+            cssVarsOnTracks: props.cssVarsOnTracks,
+            scrollTop: 0,
+            scrollLeft: 0,
+            container: this.container,
+        }
     }
 
     getSnapshotBeforeUpdate() {
@@ -129,11 +132,15 @@ export default class Scrollable extends React.PureComponent {
                 el.classList.toggle('vertically-scrollable', vRatio < 1);
                 el.classList.toggle('horizontally-scrollable', hRatio < 1);
 
-                el.style.setProperty('--scrollable-vertical-ratio', vRatio);
-                el.style.setProperty('--scrollable-horizontal-ratio', hRatio);
+                el.style.setProperty(CSS_VARS.verticalRatio, vRatio);
+                el.style.setProperty(CSS_VARS.horizontalRatio, hRatio);
+
+                this.setState(state => ({...state, scrollTop: nextEvent.top, scrollLeft: nextEvent.left}));
 
-                (this.props.cssVarsOnTracks ? this.vTrack.current : el).style.setProperty('--scrollable-scroll-top', nextEvent.top);
-                (this.props.cssVarsOnTracks ? this.hTrack.current : el).style.setProperty('--scrollable-scroll-left', nextEvent.left);
+                if (!this.props.cssVarsOnTracks) {
+                    el.style.setProperty(CSS_VARS.scrollTop, nextEvent.top);
+                    el.style.setProperty(CSS_VARS.scrollLeft, nextEvent.left);
+                }
             });
         }
 
@@ -169,9 +176,9 @@ export default class Scrollable extends React.PureComponent {
             <ResizeObserver onResize={this.updateScrollbars}>
                 <div className='scrollbar' style={style} onTransitionEnd={this.handleOnTransitionEnd}>
                     {React.cloneElement(element, this.getElementProps(), content)}
-                    <Context.Provider value={this.ctx}>
-                        {vsb ? vsb.props.children : <VerticalScrollbar xRef={this.vTrack}/>}
-                        {hsb ? hsb.props.children : <HorizontalScrollbar xRef={this.hTrack}/>}
+                    <Context.Provider value={this.state}>
+                        {vsb ? vsb.props.children : <VerticalScrollbar/>}
+                        {hsb ? hsb.props.children : <HorizontalScrollbar/>}
                     </Context.Provider>
                 </div>
             </ResizeObserver>

diff --git a/src/components/Scrollable/Scrollable.scss b/src/components/Scrollable/Scrollable.scss
@@ -1,4 +1,8 @@
 .scrollbar {
+  --scrollable-track-thickness: 12px;
+  --scrollable-thumb-thickness: Calc(var(--scrollable-track-thickness)/2);
+  --scrollable-thumb-offset: 3px;
+
   position: relative;
   max-height: 100%;
   max-width: 100%;
@@ -30,14 +34,13 @@
 
     .scrollbar-thumb {
       position: absolute;
-      padding: 3px;
       cursor: pointer;
 
       .scrollbar-thumb-inner {
         background-color: rgba(28, 34, 43, 0.6);
         border-radius: 4px;
         opacity: 0;
-        transition: opacity 0.2s ease-out 0.5s; // The transition delay is used to keep the thumb visible for a short time when the cursor leaves
+        transition: opacity 0.2s ease-out 0.5s; // The transition delay is used to keep the thumb visible for a short time when the cursor leaves. (see `Scrollable.constants.js`)
       }
     }
   }

diff --git a/src/components/Scrollable/Scrollable.test.js b/src/components/Scrollable/Scrollable.test.js
@@ -35,14 +35,14 @@ describe('<Scrollable/>', () => {
 
     describe('Life Cycle', () => {
         it('componentDidMount()', () => {
-            const s = new Scrollable();
+            const s = new Scrollable({});
             s.updateScrollbars = sinon.spy();
             s.componentDidMount();
             expect(s.updateScrollbars.calledOnce).toEqual(true);
         });
 
         it('getSnapshotBeforeUpdate()', () => {
-            const s = new Scrollable();
+            const s = new Scrollable({});
             s.container = {current: {scrollTop: 0, scrollLeft: 0}};
             expect(s.getSnapshotBeforeUpdate()).toEqual(s.container.current);
         });
@@ -114,24 +114,64 @@ describe('<Scrollable/>', () => {
             expect(onScroll.callCount).toEqual(2);
         });
 
-        it('updateScrollbars()', () => {
-            global.window.requestAnimationFrame.resetHistory();
-            const onUpdate = sinon.spy();
-            const toggle = sinon.spy();
-            const setProperty = sinon.spy();
+        describe('updateScrollbars()', () => {
+            it('updateScrollbars()', () => {
+                global.window.requestAnimationFrame.resetHistory();
+                const onUpdate = sinon.spy();
+                const toggle = sinon.spy();
+                const setProperty = sinon.spy();
 
-            const s = new Scrollable({onUpdate});
-            s.updateScrollbars();
-            expect(onUpdate.callCount).toEqual(0);
-            expect(toggle.callCount).toEqual(0);
+                const s = new Scrollable({onUpdate});
+                s.updateScrollbars();
+                expect(onUpdate.callCount).toEqual(0);
+                expect(toggle.callCount).toEqual(0);
 
-            s.container.current = {parentElement: {style: {setProperty}, classList: {toggle}}};
-            s.updateScrollbars();
-            expect(onUpdate.callCount).toEqual(1);
-            expect(global.window.requestAnimationFrame.callCount).toEqual(1);
-            global.window.requestAnimationFrame.args[0][0]();
-            expect(toggle.callCount).toEqual(2);
-            expect(setProperty.callCount).toEqual(4);
+                s.container.current = {parentElement: {style: {setProperty}, classList: {toggle}}};
+
+                // setState spy
+                const setState = jest.fn();
+                const setStateSpy = jest.spyOn(s, 'setState');
+                setStateSpy.mockImplementation(state => setState(state()));
+
+                s.updateScrollbars();
+
+                expect(onUpdate.callCount).toEqual(1);
+                expect(global.window.requestAnimationFrame.callCount).toEqual(1);
+                global.window.requestAnimationFrame.args[0][0]();
+                expect(toggle.callCount).toEqual(2);
+                expect(setProperty.callCount).toEqual(4);
+                expect(setState).toHaveBeenCalledTimes(1);
+            });
+
+            it('should not add CSS variables on the component', () => {
+                global.window.requestAnimationFrame.resetHistory();
+                const toggle = sinon.spy();
+                const containerSetProperty = sinon.spy();
+
+                const s = new Scrollable({onUpdate: noop, cssVarsOnTracks:true});
+                s.container.current = {parentElement: {style: {setProperty: containerSetProperty}, classList: {toggle}}};
+                s.event = {next: {top:5, left:10}, prev: {top:0, left:0}};
+
+                // setState spy
+                const setState = jest.fn();
+                const setStateSpy = jest.spyOn(s, 'setState');
+                setStateSpy.mockImplementation(state => setState(state()));
+
+                s.updateScrollbars();
+
+                expect(global.window.requestAnimationFrame.callCount).toEqual(1);
+                global.window.requestAnimationFrame.args[0][0]();
+                expect(toggle.callCount).toEqual(2);
+                expect(containerSetProperty.callCount).toEqual(2);
+
+                // expect(s.setState.calledWith( sinon.match(newState) )).toEqual(true);
+                expect(setState).toHaveBeenCalledWith(
+                    expect.objectContaining({
+                        scrollTop : s.event.next.top,
+                        scrollLeft: s.event.next.left,
+                    })
+                );
+            });
         });
 
         // makes sure the change was detected the the re-calc in requestAnimationFrame is fired
@@ -146,7 +186,7 @@ describe('<Scrollable/>', () => {
         });
 
         it('onTransitionEnd()', () => {
-            const s = new Scrollable();
+            const s = new Scrollable({});
             s.updateScrollbars = sinon.spy();
             s.handleOnTransitionEnd({propertyName: 'foo'});
             expect(s.updateScrollbars.callCount).toEqual(0);
@@ -167,33 +207,4 @@ describe('<Scrollable/>', () => {
             expect(normalizeScrollPosition(2000, 1000, 333)).toEqual(0.333);
         });
     });
-
-    describe('Props', () => {
-        describe('cssVarsOnTracks', () => {
-            it('updateScrollbars()', () => {
-                global.window.requestAnimationFrame.resetHistory();
-                const toggle = sinon.spy();
-                const containerSetProperty = sinon.spy();
-                const hTrackSetProperty = sinon.spy();
-                const vTrackSetProperty = sinon.spy();
-
-                const s = new Scrollable({onUpdate: noop, cssVarsOnTracks:true});
-                s.container.current = {parentElement: {style: {setProperty: containerSetProperty}, classList: {toggle}}};
-                s.hTrack.current = {style: {setProperty: hTrackSetProperty}};
-                s.vTrack.current = {style: {setProperty: vTrackSetProperty}};
-                s.event = {next: {top:5, left:10}, prev: {top:0, left:0}};
-
-                s.updateScrollbars();
-                expect(global.window.requestAnimationFrame.callCount).toEqual(1);
-                global.window.requestAnimationFrame.args[0][0]();
-                expect(toggle.callCount).toEqual(2);
-                expect(containerSetProperty.callCount).toEqual(2);
-                expect(hTrackSetProperty.callCount).toEqual(1);
-                expect(vTrackSetProperty.callCount).toEqual(1);
-
-                expect(hTrackSetProperty.calledWith('--scrollable-scroll-left', s.event.next.left)).toEqual(true);
-                expect(vTrackSetProperty.calledWith('--scrollable-scroll-top', s.event.next.top)).toEqual(true);
-            });
-        });
-    });
 });
diff --git a/src/components/Scrollable/components/HorizontalScrollbar/HorizontalScrollbar.jsx b/src/components/Scrollable/components/HorizontalScrollbar/HorizontalScrollbar.jsx
@@ -17,15 +17,14 @@
 import React, {useContext, useMemo, useRef} from 'react';
 import Movable from 'components/Movable';
 import Context from '../../Scrollable.context';
+import {CSS_VARS} from '../../Scrollable.constants';
 import {move} from './HorizontalScrollbar.operations';
 import './HorizontalScrollbar.scss';
-import {propTypes} from '../VerticalScrollbar/VerticalScrollbar.props';
 
-const HorizontalScrollbar = ({xRef}) => {
-    let track = useRef();
-    track = xRef || track;
+const HorizontalScrollbar = () => {
+    const track = useRef();
     const thumb = useRef();
-    const {container} = useContext(Context);
+    const {container, scrollLeft, cssVarsOnTracks} = useContext(Context);
     const props = Movable.useMove(useMemo(() => [move(container, thumb, track)], [container]));
 
     const handleOnClick = e => {
@@ -42,14 +41,12 @@ const HorizontalScrollbar = ({xRef}) => {
     };
 
     return (
-        <div className='scrollbar-track horizontal-scrollbar-track' ref={track} onClick={handleOnClick}>
+        <div className='scrollbar-track horizontal-scrollbar-track' style={cssVarsOnTracks ? {[CSS_VARS.scrollLeft]: scrollLeft} : undefined} ref={track} onClick={handleOnClick}>
             <Movable className='scrollbar-thumb' ref={thumb} {...props}>
                 <div className='scrollbar-thumb-inner'/>
             </Movable>
         </div>
     );
 };
 
-HorizontalScrollbar.propTypes = propTypes;
-
-export default HorizontalScrollbar;
+export default HorizontalScrollbar;
diff --git a/src/components/Scrollable/components/HorizontalScrollbar/HorizontalScrollbar.scss b/src/components/Scrollable/components/HorizontalScrollbar/HorizontalScrollbar.scss
@@ -1,27 +1,3 @@
-.scrollbar {
-  &.horizontally-scrollable .horizontal-scrollbar-track {
-    visibility: visible;
-    pointer-events: auto;
-  }
+@import '../tracks';
 
-  .horizontal-scrollbar-track {
-    position: absolute;
-    height: 12px;
-    width: 100%;
-    bottom: 0;
-    left: 0;
-
-    .scrollbar-thumb {
-      --thumb-width: max(calc((var(--scrollable-horizontal-ratio, 1) * 100%)), var(--scrollable-min-thumb-length, 30px));
-      bottom: 0;
-      box-sizing: border-box;
-      left: calc(var(--scrollable-scroll-left, 0) * (100% - var(--thumb-width)));
-      width: var(--thumb-width);
-
-      .scrollbar-thumb-inner {
-        height: 6px;
-        width: 100%;
-      }
-    }
-  }
-}
+@include tracks(false);
diff --git a/src/components/Scrollable/components/HorizontalScrollbar/HorizontalScrollbar.test.js b/src/components/Scrollable/components/HorizontalScrollbar/HorizontalScrollbar.test.js
@@ -2,6 +2,7 @@ import React from 'react';
 import {mount, shallow} from 'enzyme';
 import {noop} from 'utility/memory';
 import {move} from './HorizontalScrollbar.operations';
+import {CSS_VARS} from '../../Scrollable.constants';
 import HorizontalScrollbar, {__RewireAPI__ as rewire} from './HorizontalScrollbar';
 
 describe('<HorizontalScrollbar/>', () => {
@@ -11,6 +12,19 @@ describe('<HorizontalScrollbar/>', () => {
             const wrapper = mount(<HorizontalScrollbar/>);
             expect(wrapper.find('.scrollbar-thumb')).toHaveLength(2);
             expect(wrapper.find('.scrollbar-thumb-inner')).toHaveLength(1);
+            expect(wrapper.find('.scrollbar-track').prop('style')).toBeUndefined();
+        });
+
+        it('should have correct style with CSS variable', () => {
+            const context = {
+                container: {},
+                scrollLeft: 10,
+                cssVarsOnTracks: true,
+            };
+            rewire.__Rewire__('useContext', () => context);
+
+            const wrapper = shallow(<HorizontalScrollbar/>);
+            expect(wrapper.find('.scrollbar-track').prop('style')).toEqual({[CSS_VARS.scrollLeft]: context.scrollLeft});
         });
     });
 
@@ -31,6 +45,20 @@ describe('<HorizontalScrollbar/>', () => {
             rewire.__ResetDependency__('useRef');
             rewire.__ResetDependency__('useContext');
         });
+
+        it('handleOnClick() on thumb', () => {
+            const container = {current: {style: {}, scrollLeft: 0, scrollHeight: 200}};
+            const ref = {current: {contains: () => true, getBoundingClientRect: () => ({left: 0, width: 100})}};
+            rewire.__Rewire__('useRef', () => ref);
+            rewire.__Rewire__('useContext', () => ({container}));
+            const wrapper = shallow(<HorizontalScrollbar/>);
+
+            wrapper.find('.scrollbar-track').prop('onClick')({clientX: 50, stopPropagation: noop});
+            expect(container.current.scrollLeft).toEqual(0);
+
+            rewire.__ResetDependency__('useRef');
+            rewire.__ResetDependency__('useContext');
+        });
     });
 
     describe('Operations', () => {

diff --git a/src/components/Scrollable/components/VerticalScrollbar/VerticalScrollbar.jsx b/src/components/Scrollable/components/VerticalScrollbar/VerticalScrollbar.jsx
@@ -17,15 +17,14 @@
 import React, {useContext, useMemo, useRef} from 'react';
 import Movable from 'components/Movable';
 import Context from '../../Scrollable.context';
+import {CSS_VARS} from '../../Scrollable.constants';
 import {move} from './VerticalScrollbar.operations';
 import './VerticalScrollbar.scss';
-import {propTypes} from './VerticalScrollbar.props';
 
-const VerticalScrollbar = ({xRef}) => {
-    let track = useRef();
-    track = xRef || track;
+const VerticalScrollbar = () => {
+    const track = useRef();
     const thumb = useRef();
-    const {container} = useContext(Context);
+    const {container, scrollTop, cssVarsOnTracks} = useContext(Context);
     const props = Movable.useMove(useMemo(() => [move(container, thumb, track)], [container]));
 
     const handleOnClick = e => {
@@ -42,14 +41,12 @@ const VerticalScrollbar = ({xRef}) => {
     };
 
     return (
-        <div className='scrollbar-track vertical-scrollbar-track' ref={track} onClick={handleOnClick}>
+        <div className='scrollbar-track vertical-scrollbar-track' style={cssVarsOnTracks ? {[CSS_VARS.scrollTop]: scrollTop} : undefined} ref={track} onClick={handleOnClick}>
             <Movable className='scrollbar-thumb' ref={thumb} {...props}>
                 <div className='scrollbar-thumb-inner'/>
             </Movable>
         </div>
     );
 };
 
-VerticalScrollbar.propTypes = propTypes;
-
-export default VerticalScrollbar;
+export default VerticalScrollbar;