Dana rm32151 scroll fix remarks from initial impl review

package.json

@@ -47,6 +47,7 @@
     "@storybook/addon-essentials": "^6.3.11",
     "@storybook/addon-links": "^6.3.11",
     "@storybook/addon-storysource": "^6.5.12",
+    "@storybook/api": "^6.3.11",
     "@storybook/react": "^6.3.11",
     "@vitejs/plugin-react": "^4.0.3",
     "antd": "^3.6.5",

src/components/Scrollbar.tsx

@@ -3,72 +3,65 @@ import Measure from 'react-measure';
 
 export interface ScrollbarProperties {
     /**
-     * A number which represents the maximum scroll position
+     * E.g. 0
      */
     minScrollPosition: number;
 
     /**
-     * A number which represents the minimum scroll position
+     * E.g. 100
      */
     maxScrollPosition: number;
 
     /**
-     * The number of lines equivalent to one page.
+     * E.g. 10
      */
     pageSize: number;
 
     /**
-     * The initial position of the scroll thumb
+     * E.g. W/ the previous examples, can have values between 0 and 90
      */
-    scrollPosition?: number;
+    initialScrollPosition?: number;
 
     /**
      * Can be both `Direction.HORIZONTAL`(the default value) or `Direction.VERTICAL`
      */
     direction?: Direction;
-
-    /**
-     * By default Semantic ui hides the increase/decrease arrow buttons of the scrollbars on Webkit based browsers. So it default to false.
-     * This should be set to true if the scrollbar needs to display the arrow buttons.
-     */
-    hasArrows?: boolean;
 
     /**
-     * This handler is called when the scrollbar is scrolled.
-     * It receives as parameter the current scrollPosition
      * This handler can be used for example to synchronize the scrollbar position with a virtual scroll of another component
      *
-     *  @param scrollPosition 
-     * @returns 
+     * E.g. W/ the previous examples, the scrollPosition can be between 0 and 90
      */
     onScroll?: (scrollPosition: number) => void;
 
-    /**
-     * If the configuration of the scrollbar doesn't require scrolling (i.e. the min-max interval is smaller than the pageSize)
-     * the Scrollbar component doesn't display nothing. 
-     * 
-     * This handler can be used to detect when the scrollbar is not required to be displayed
-     * 
-     * @param isScrollbarVisible 
-     * @returns 
-     */
-    onVisibilityChange?: (isScrollbarVisible: boolean) => void;
+   /* 
+    * The scrollbar component already measures its content. 
+    * So the parent component should use this handler
+    * if it needs to detect scrollbar size  changes (e.g. when the component becomes invisible 
+    * because the current configuration needs no scrollbar)
+    */
+    onResize?: (scrollbarContentRect: boolean) => void;
 }
 
 export enum Direction {
-    HORIZONTAL,
-    VERTICAL
+    HORIZONTAL = "horizontal",
+    VERTICAL = "vertical"
 }
 
-export const SCROLLBAR_SIZE = 10;
 const ROUND_FACTOR = 4;
 
+/**
+ * If the configuration of the scrollbar doesn't require scrolling 
+ * (i.e. the min-max interval is smaller than the pageSize)
+ * the Scrollbar component doesn't display nothing. 
+ * 
+ * @author Daniela Buzatu
+ */
 export class Scrollbar extends React.Component<ScrollbarProperties, { scrollbarSize: number }> {
 
     static defaultProps = {
         scrollPosition: 0,
-        direction: Direction.HORIZONTAL,
-        hasArrows: false
+        direction: Direction.HORIZONTAL
     }
 
     _outterDiv: HTMLDivElement;
@@ -101,21 +94,11 @@ export class Scrollbar extends React.Component<ScrollbarProperties, { scrollbarS
         this.setScrollPositionInPx((newScrollPosition - this.props.minScrollPosition) * pixels_per_unit);
     }
 
-    componentDidMount(): void {
-        if (this.props.onVisibilityChange) {
-            this.props.onVisibilityChange(this.isScrollbarNeeded(this.props));
-        } 
-    }
-
     componentWillReceiveProps(nextProps: Readonly<ScrollbarProperties>): void {
-        if (nextProps.onVisibilityChange && this.isScrollbarNeeded(nextProps) != this.isScrollbarNeeded(this.props)) {
-            nextProps.onVisibilityChange(this.isScrollbarNeeded(nextProps));
-        } 
-
         if (nextProps.minScrollPosition != this.props.minScrollPosition || 
             nextProps.maxScrollPosition != this.props.maxScrollPosition ||
             nextProps.pageSize != this.props.pageSize ||
-            nextProps.scrollPosition != this.props.scrollPosition) { 
+            nextProps.initialScrollPosition != this.props.initialScrollPosition) { 
             this.setScrollPosition(nextProps);
         }
     }
@@ -128,7 +111,7 @@ export class Scrollbar extends React.Component<ScrollbarProperties, { scrollbarS
 
     setScrollPosition(props: ScrollbarProperties) {
         const pixels_per_unit = this.state.scrollbarSize / props.pageSize;
-       this.setScrollPositionInPx((props.scrollPosition - props.minScrollPosition) * pixels_per_unit);
+       this.setScrollPositionInPx((props.initialScrollPosition - props.minScrollPosition) * pixels_per_unit);
     }
 
     setScrollPositionInPx(scrollPositionInPixels) {
@@ -140,7 +123,7 @@ export class Scrollbar extends React.Component<ScrollbarProperties, { scrollbarS
         // a scrollbar that is "double binded" to a state variable in the parent component (e.g. in gantt)
         // It is caused by a small precision lost when converting from pixels to client unit and then back again
         const oldScrollPositionInPixels = this.props.direction == Direction.HORIZONTAL ? this._outterDiv.scrollLeft : this._outterDiv.scrollTop;
-        if (roundDoubleToDecimals(scrollPositionInPixels) == roundDoubleToDecimals(oldScrollPositionInPixels)) {
+        if (this.roundDoubleToDecimals(scrollPositionInPixels) == this.roundDoubleToDecimals(oldScrollPositionInPixels)) {
             return;
         }
 
@@ -167,8 +150,15 @@ export class Scrollbar extends React.Component<ScrollbarProperties, { scrollbarS
     }
 
     getOutterDivClassName() {
-        let className = (this.props.direction == Direction.HORIZONTAL ? "rct9k-horizontal-scrollbar-outter" : "rct9k-vertical-scrollbar-outter");
-        return className + (this.props.hasArrows ? " rct9k-scrollbar-with-arrows" : "");
+        return this.props.direction == Direction.HORIZONTAL ? "rct9k-horizontal-scrollbar-outter" : "rct9k-vertical-scrollbar-outter";
+    }
+
+    roundDoubleToDecimals = (value: number) => {
+        if (value == 0) {
+            return value;
+        }
+        let roundFactor = Math.pow(10, ROUND_FACTOR);
+        return Math.round(value * roundFactor) / roundFactor;
     }
 
     render(): React.ReactNode {
@@ -179,13 +169,13 @@ export class Scrollbar extends React.Component<ScrollbarProperties, { scrollbarS
                 if (newSize != this.state.scrollbarSize) {
                     this.setState({ scrollbarSize: newSize });
                 }
+                this.props.onResize && this.props.onResize(contentRect);
             }}>
             {({ measureRef }) => {
                 return (
                     this.isScrollbarNeeded(this.props) ?
                     <div
                         className={this.getOutterDivClassName()}
-                        style={this.props.direction == Direction.HORIZONTAL ? {height: SCROLLBAR_SIZE} : {width: SCROLLBAR_SIZE}}
                         ref={(node) => {
                             measureRef(node);
                             this._outterDiv = node;
@@ -202,12 +192,4 @@ export class Scrollbar extends React.Component<ScrollbarProperties, { scrollbarS
             }}
         </Measure>
     }
-}
-
-const roundDoubleToDecimals = (value: number) => {
-    if (value == 0) {
-        return value;
-    }
-    let roundFactor = Math.pow(10, ROUND_FACTOR);
-    return Math.round(value * roundFactor) / roundFactor;
 }

src/stories/basic/Basic.stories.tsx

@@ -101,33 +101,15 @@ AlternativeRowColoring.parameters = {
 };
 
 export const HorizontalScroll = () => {
-const tasks: Item[] = [
-    ...someTasks, // Tasks that are outside the display interval  
-    { key: 11, row: 4, title: 'Task GW1', start: d('2018-09-19 7:00'), end: d('2018-09-19 8:00') },
-    { key: 12, row: 4, title: 'Task GW2', start: d('2018-09-19 17:00'), end: d('2018-09-19 19:00') },
-	  { key: 14, row: 1, title: 'Task GW3', start: d('2018-09-21 12:00'), end: d('2018-09-21 14:00') },
-    { key: 15, row: 1, title: 'Task GW4', start: d('2018-09-21 15:00'), end: d('2018-09-21 17:00') },
-    { key: 16, row: 1, title: 'Task GW5', start: d('2018-09-19 12:00'), end: d('2018-09-19 14:00') },
-    { key: 17, row: 1, title: 'Task GW6', start: d('2018-09-19 9:00'), end: d('2018-09-19 12:00') },
-  ];
-
   return (
-    <>
-      {/* This is a trivial example to illustrate how Timeline "glues" to its "flex" parent. Notes: */}
-      {/* 1/ In other stories we don't have this, because we have a Storybook decorator that wraps w/ a div + CSS class. */}
-      {/* 2/ You'll probably have a better flex-box layout, i.e. not hardcoded. 3/ Use CSS classes and not styles. */}
-      <div style={{ display: 'flex', height: '400px' }}>
-        <Timeline startDate={d('2018-09-20')} endDate={d('2018-09-21')} minDate={d('2018-09-19')} maxDate={d('2018-09-22')} groups={someHumanResources} items={tasks}
-                  table={<Table width={100} >
-                            <Column
-                                columnKey="title"
-                                width={100}
-                                header={<DataCell>Title</DataCell>}
-                                cell={({rowIndex}) => <DataCell>{rowIndex < someHumanResources.length ? someHumanResources[rowIndex].title : ""}</DataCell>}/>
-                        </Table>}
-          />
-      </div>
-    </>
+    <Timeline startDate={d('2018-09-20')} endDate={d('2018-09-21')} minDate={d('2018-09-19')} maxDate={d('2018-09-22')} groups={someHumanResources} items={someTasks}
+              table={<Table width={100} >
+                        <Column
+                            columnKey="title"
+                            width={100}
+                            header={<DataCell>Title</DataCell>}
+                            cell={({rowIndex}) => <DataCell>{rowIndex < someHumanResources.length ? someHumanResources[rowIndex].title : ""}</DataCell>}/>
+                    </Table>}/>
   );
 };
 

src/stories/sampleData.ts

@@ -40,7 +40,14 @@ export const someTasks: Item[] = [
   {key: 8, row: 2, title: 'Task MD5', start: d('2018-09-20 20:00'), end: d('2018-09-20 21:00')},
   {key: 9, row: 2, title: 'Task MD2', start: d('2018-09-20 5:00'), end: d('2018-09-20 7:00')},
   {key: 10, row: 2, title: 'Task MD3', start: d('2018-09-20 13:00'), end: d('2018-09-20 14:00')},
-  {key: 11, row: 2, title: 'Task MD3', start: d('2018-09-20 22:00'), end: d('2018-09-20 24:00')}
+  {key: 11, row: 2, title: 'Task MD3', start: d('2018-09-20 22:00'), end: d('2018-09-20 24:00')},
+  // Tasks that are outside the display interval 
+  { key: 12, row: 4, title: 'Task GW1', start: d('2018-09-19 7:00'), end: d('2018-09-19 8:00') },
+  { key: 13, row: 4, title: 'Task GW2', start: d('2018-09-19 17:00'), end: d('2018-09-19 19:00') },
+  { key: 14, row: 1, title: 'Task GW3', start: d('2018-09-21 12:00'), end: d('2018-09-21 14:00') },
+  { key: 15, row: 1, title: 'Task GW4', start: d('2018-09-21 15:00'), end: d('2018-09-21 17:00') },
+  { key: 16, row: 1, title: 'Task GW5', start: d('2018-09-19 12:00'), end: d('2018-09-19 14:00') },
+  { key: 17, row: 1, title: 'Task GW6', start: d('2018-09-19 9:00'), end: d('2018-09-19 12:00') }
 ];
 
 export const manyHumanResources: Employee[] = [...someHumanResources, { id: 4, title: 'George Walsh', job: 'Developer' }, 

src/stories/scrollbar/Scrollbar.stories.tsx

@@ -3,12 +3,69 @@ import { Direction, Scrollbar } from "../../components/Scrollbar";
 import { useRef, useState } from "react";
 import { Alert } from "antd";
 import Measure from 'react-measure';
+import { createTestids } from "@famiprog-foundation/tests-are-demo";
+import { useArgs } from '@storybook/client-api';
+
 
 export default {
-    title: 'Components/Scrollbar'
+    title: 'Components/Scrollbar',
+    component: Scrollbar,
+    includeStories: /^[A-Z]/,
+    parameters: {
+        controls: {
+          disable: false
+        }
+    },
+    args: {
+        pageSize: 10,
+        minScrollPosition: 0,
+        maxScrollPosition: 100,
+        initialScrollPosition: 0,
+        scrollPosition: 0
+    },
+    argTypes: {
+        onScroll: {
+            table: {
+                disable: true,
+              }
+        },
+        onVisibilityChange: {
+            table: {
+                disable: true,
+              }
+        }
+    }
 };
 
-export const HorizontalScrollBar = () => {
+export const scrollbarTestIds = createTestids('ScrollbarStory', {
+    scrollbar: ''
+});
+
+export const HorizontalScrollBar = (args) => {
+    const [{}, updateArgs] = useArgs();
+    return(
+        <div style={{marginTop: 30, display: "flex"}}>
+            <Scrollbar onScroll={(scrollPosition) => updateArgs({scrollPosition: scrollPosition})} {...args}/>
+        </div>
+    );
+}
+HorizontalScrollBar.args = {
+    direction: Direction.HORIZONTAL
+}
+
+export const VerticalScrollBar = (args) => {
+    const [{}, updateArgs] = useArgs();
+    return (
+        <div style={{marginLeft: 30, display: "flex", height:"100%"}}>
+            <Scrollbar onScroll={(scrollPosition) => updateArgs({scrollPosition: scrollPosition})} {...args}/>
+        </div>
+    );
+}
+VerticalScrollBar.args = {
+    direction: Direction.VERTICAL
+}
+
+export const HorizontalScrollBarScrollingADiv = () => {
     const divContentWidth1 = 700;
     const divContentWidth2 = 700;
     const totalWidth = 800;
@@ -62,14 +119,20 @@ export const HorizontalScrollBar = () => {
                     <Scrollbar pageSize={divWidth1} minScrollPosition={0} maxScrollPosition={divContentWidth1}
                         onScroll={(scrollPosition) => div1.current.scrollLeft = scrollPosition}/>
                     <Scrollbar pageSize={divWidth2} minScrollPosition={0} maxScrollPosition={divContentWidth2}
-                        onScroll={(scrollPosition) => div2.current.scrollLeft = scrollPosition} hasArrows={true}/>
+                        onScroll={(scrollPosition) => div2.current.scrollLeft = scrollPosition}/>
                 </SplitPane>
             </div>
         </>
     );
 }
 
-export const VerticalScrollBar = () => {
+HorizontalScrollBarScrollingADiv.parameters = {
+    controls: {
+      disable: true
+    }
+};
+
+export const VerticalScrollBarScrollingADiv = () => {
     const divContentHeight1 = 500;
     const divContentHeight2 = 500;
     const totalHeight = 500;
@@ -124,10 +187,16 @@ export const VerticalScrollBar = () => {
                         <Scrollbar direction={Direction.VERTICAL} pageSize={divHeight1} minScrollPosition={0} maxScrollPosition={divContentHeight1}
                             onScroll={(scrollPosition) => div1.current.scrollTop = scrollPosition}/>
                         <Scrollbar direction={Direction.VERTICAL} pageSize={divHeight2} minScrollPosition={0} maxScrollPosition={divContentHeight2}
-                            onScroll={(scrollPosition) => div2.current.scrollTop = scrollPosition} hasArrows={true}/>
+                            onScroll={(scrollPosition) => div2.current.scrollTop = scrollPosition}/>
                     </SplitPane>
                 </div>
             </div>
         </>
     );
-}
+}
+
+VerticalScrollBarScrollingADiv.parameters = {
+    controls: {
+      disable: true
+    }
+};