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

Author: yairEO

src/components/Scrollable/Scrollable.jsx

@@ -113,15 +113,15 @@ export default class Scrollable extends React.PureComponent {
         const nextEvent = this.event.next,
             prevEvent = this.event.prev,
             changed = nextEvent.scrollHeight !== prevEvent.scrollHeight ||
-            nextEvent.scrollWidth !== prevEvent.scrollWidth ||
-            nextEvent.top !== prevEvent.top ||
-            nextEvent.left !== prevEvent.left;
+                      nextEvent.scrollWidth !== prevEvent.scrollWidth ||
+                      nextEvent.top !== prevEvent.top ||
+                      nextEvent.left !== prevEvent.left;
 
         // Ensures that updates (which are a potentially expensive operation)
         // are only executed if the applicable scroll properties have changed
         if (changed) {
-            const el = this.container.current.parentElement;
             this.props.onUpdate(nextEvent);
+            const el = this.container.current.parentElement;
             const vRatio = nextEvent.clientHeight / nextEvent.scrollHeight;
             const hRatio = nextEvent.clientWidth / nextEvent.scrollWidth;
 
@@ -132,14 +132,8 @@ export default class Scrollable extends React.PureComponent {
                 el.style.setProperty('--scrollable-vertical-ratio', vRatio);
                 el.style.setProperty('--scrollable-horizontal-ratio', hRatio);
 
-                if( this.props.cssVarsOnTracks ) {
-                    this.vTrack.current.style.setProperty('--scrollable-scroll-top', nextEvent.top);
-                    this.hTrack.current.style.setProperty('--scrollable-scroll-left', nextEvent.left);
-                }
-                else {
-                    el.style.setProperty('--scrollable-scroll-top', nextEvent.top);
-                    el.style.setProperty('--scrollable-scroll-left', 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);
             });
         }
 

src/components/Scrollable/Scrollable.test.js

@@ -2,7 +2,7 @@ import React from 'react';
 import {mount} from 'enzyme';
 import sinon from 'sinon';
 import {noop} from 'utility/memory';
-import Scrollable from './Scrollable';
+import Scrollable from './';
 import {normalizeScrollPosition} from './Scrollable.utils';
 import {SCROLLING_CLASS_REMOVAL_DELAY} from './Scrollable.constants';
 
@@ -18,6 +18,19 @@ describe('<Scrollable/>', () => {
             expect(wrapper.find('VerticalScrollbar')).toHaveLength(1);
             expect(wrapper.find('HorizontalScrollbar')).toHaveLength(1);
         });
+
+        it('should render a Scrollbar with custom vertical & horizontal scrollbars', () => {
+            const wrapper = mount(
+                <Scrollable>
+                    <Scrollable.VerticalScrollbar><div className='vsb-child'></div></Scrollable.VerticalScrollbar>
+                    <Scrollable.HorizontalScrollbar><div className='hsb-child'></div></Scrollable.HorizontalScrollbar>
+                </Scrollable>
+            );
+            expect(wrapper.find('.scrollbar')).toHaveLength(1);
+            expect(wrapper.find('ResizeObserver')).toHaveLength(1);
+            expect(wrapper.find('.vsb-child')).toHaveLength(1);
+            expect(wrapper.find('.hsb-child')).toHaveLength(1);
+        });
     });
 
     describe('Life Cycle', () => {
@@ -34,14 +47,26 @@ describe('<Scrollable/>', () => {
             expect(s.getSnapshotBeforeUpdate()).toEqual(s.container.current);
         });
 
-        it('componentDidUpdate()', () => {
-            const s = new Scrollable({scrollOnDOMChange: false});
-            s.container = {current: {scrollTop: 0}};
-            s.updateScrollbars = sinon.spy();
-            s.componentDidUpdate(null, null, {scrollTop: 50, scrollLeft: 50});
-            expect(s.container.current.scrollTop).toEqual(50);
-            expect(s.container.current.scrollLeft).toEqual(50);
-            expect(s.updateScrollbars.callCount).toEqual(1);
+        describe('componentDidUpdate', () => {
+            it('scrollOnDOMChange prop "true"', () => {
+                const s = new Scrollable({scrollOnDOMChange: true});
+                s.container = {current: {scrollTop: 0, scrollLeft: 0}};
+                s.updateScrollbars = sinon.spy();
+                s.componentDidUpdate(null, null, {scrollTop: 50, scrollLeft: 50});
+                expect(s.container.current.scrollTop).toEqual(0);
+                expect(s.container.current.scrollLeft).toEqual(0);
+                expect(s.updateScrollbars.callCount).toEqual(1);
+            });
+
+            it('scrollOnDOMChange prop "false"', () => {
+                const s = new Scrollable({scrollOnDOMChange: false});
+                s.container = {current: {scrollTop: 0}};
+                s.updateScrollbars = sinon.spy();
+                s.componentDidUpdate(null, null, {scrollTop: 50, scrollLeft: 50});
+                expect(s.container.current.scrollTop).toEqual(50);
+                expect(s.container.current.scrollLeft).toEqual(50);
+                expect(s.updateScrollbars.callCount).toEqual(1);
+            });
         });
     });
 
@@ -162,7 +187,6 @@ describe('<Scrollable/>', () => {
                 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);