Grid takes scrollbar size into account when aligning cells for scroll…

…ToColumn or scrollToRow usage.
bvaughn · Feb 21, 2017 · 08d4227 · 08d4227
1 parent 0616c2e
commit 08d4227
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 17 deletions.
diff --git a/source/Grid/Grid.jest.js b/source/Grid/Grid.jest.js
@@ -1,4 +1,3 @@
-import getScrollbarSize from 'dom-helpers/util/scrollbarSize'
 import React from 'react'
 import { findDOMNode } from 'react-dom'
 import { Simulate } from 'react-addons-test-utils'
@@ -15,6 +14,14 @@ const DEFAULT_WIDTH = 200
 const NUM_ROWS = 100
 const NUM_COLUMNS = 50
 
+function getScrollbarSize0 () {
+  return 0
+}
+
+function getScrollbarSize20 () {
+  return 20
+}
+
 describe('Grid', () => {
   function defaultCellRenderer ({ columnIndex, key, rowIndex, style }) {
     return (
@@ -44,6 +51,7 @@ describe('Grid', () => {
         cellRenderer={defaultCellRenderer}
         columnCount={NUM_COLUMNS}
         columnWidth={DEFAULT_COLUMN_WIDTH}
+        getScrollbarSize={getScrollbarSize0}
         height={DEFAULT_HEIGHT}
         overscanColumnCount={0}
         overscanRowCount={0}
@@ -105,15 +113,10 @@ describe('Grid', () => {
   })
 
   describe('shows and hides scrollbars based on rendered content', () => {
-    let scrollbarSize
-
-    beforeAll(() => {
-      scrollbarSize = getScrollbarSize()
-    })
-
     it('should set overflowX:hidden if columns fit within the available width and y-axis has no scrollbar', () => {
       const rendered = findDOMNode(render(getMarkup({
         columnCount: 4,
+        getScrollbarSize: getScrollbarSize20,
         rowCount: 5
       })))
       expect(rendered.style.overflowX).toEqual('hidden')
@@ -122,14 +125,16 @@ describe('Grid', () => {
     it('should set overflowX:hidden if columns and y-axis scrollbar fit within the available width', () => {
       const rendered = findDOMNode(render(getMarkup({
         columnCount: 4,
-        width: 200 + scrollbarSize
+        getScrollbarSize: getScrollbarSize20,
+        width: 200 + getScrollbarSize20()
       })))
       expect(rendered.style.overflowX).toEqual('hidden')
     })
 
     it('should leave overflowX:auto if columns require more than the available width', () => {
       const rendered = findDOMNode(render(getMarkup({
         columnCount: 4,
+        getScrollbarSize: getScrollbarSize20,
         width: 200 - 1,
         rowCount: 5
       })))
@@ -139,13 +144,15 @@ describe('Grid', () => {
     it('should leave overflowX:auto if columns and y-axis scrollbar require more than the available width', () => {
       const rendered = findDOMNode(render(getMarkup({
         columnCount: 4,
-        width: 200 + scrollbarSize - 1
+        getScrollbarSize: getScrollbarSize20,
+        width: 200 + getScrollbarSize20() - 1
       })))
       expect(rendered.style.overflowX).not.toEqual('hidden')
     })
 
     it('should set overflowY:hidden if rows fit within the available width and xaxis has no scrollbar', () => {
       const rendered = findDOMNode(render(getMarkup({
+        getScrollbarSize: getScrollbarSize20,
         rowCount: 5,
         columnCount: 4
       })))
@@ -154,14 +161,16 @@ describe('Grid', () => {
 
     it('should set overflowY:hidden if rows and x-axis scrollbar fit within the available width', () => {
       const rendered = findDOMNode(render(getMarkup({
+        getScrollbarSize: getScrollbarSize20,
         rowCount: 5,
-        height: 100 + scrollbarSize
+        height: 100 + getScrollbarSize20()
       })))
       expect(rendered.style.overflowY).toEqual('hidden')
     })
 
     it('should leave overflowY:auto if rows require more than the available width', () => {
       const rendered = findDOMNode(render(getMarkup({
+        getScrollbarSize: getScrollbarSize20,
         rowCount: 5,
         height: 100 - 1,
         columnCount: 4
@@ -171,15 +180,17 @@ describe('Grid', () => {
 
     it('should leave overflowY:auto if rows and x-axis scrollbar require more than the available width', () => {
       const rendered = findDOMNode(render(getMarkup({
+        getScrollbarSize: getScrollbarSize20,
         rowCount: 5,
-        height: 100 + scrollbarSize - 1
+        height: 100 + getScrollbarSize20() - 1
       })))
       expect(rendered.style.overflowY).not.toEqual('hidden')
     })
 
     it('should accept styles that overwrite calculated ones', () => {
       const rendered = findDOMNode(render(getMarkup({
         columnCount: 1,
+        getScrollbarSize: getScrollbarSize20,
         height: 1,
         rowCount: 1,
         style: {
@@ -412,6 +423,23 @@ describe('Grid', () => {
       expect(grid.state.scrollLeft).toEqual(2450)
       expect(grid.state.scrollTop).toEqual(920)
     })
+
+    it('should take scrollbar size into account when aligning cells', () => {
+      const grid = render(getMarkup({
+        columnWidth: 50,
+        columnCount: 100,
+        getScrollbarSize: getScrollbarSize20,
+        height: 100,
+        rowCount: 100,
+        rowHeight: 20,
+        scrollToColumn: 50,
+        scrollToRow: 50,
+        width: 100
+      }))
+
+      expect(grid.state.scrollLeft).toEqual(2450 + getScrollbarSize20())
+      expect(grid.state.scrollTop).toEqual(920 + getScrollbarSize20())
+    })
   })
 
   describe('property updates', () => {

diff --git a/source/Grid/Grid.js b/source/Grid/Grid.js
@@ -5,7 +5,6 @@ import calculateSizeAndPositionDataAndUpdateScrollOffset from './utils/calculate
 import ScalingCellSizeAndPositionManager from './utils/ScalingCellSizeAndPositionManager'
 import createCallbackMemoizer from '../utils/createCallbackMemoizer'
 import defaultOverscanIndicesGetter, { SCROLL_DIRECTION_BACKWARD, SCROLL_DIRECTION_FORWARD } from './utils/defaultOverscanIndicesGetter'
-import getScrollbarSize from 'dom-helpers/util/scrollbarSize'
 import updateScrollIndexHelper from './utils/updateScrollIndexHelper'
 import defaultCellRangeRenderer from './defaultCellRangeRenderer'
 
@@ -105,6 +104,11 @@ export default class Grid extends PureComponent {
      */
     estimatedRowSize: PropTypes.number.isRequired,
 
+    /**
+     * Exposed for testing purposes only.
+     */
+    getScrollbarSize: PropTypes.func.isRequired,
+
     /**
      * Height of Grid; this property determines the number of visible (vs virtualized) rows.
      */
@@ -212,6 +216,7 @@ export default class Grid extends PureComponent {
     cellRangeRenderer: defaultCellRangeRenderer,
     estimatedColumnSize: 100,
     estimatedRowSize: 30,
+    getScrollbarSize: require('dom-helpers/util/scrollbarSize'),
     noContentRenderer: () => null,
     onScroll: () => null,
     onSectionRendered: () => null,
@@ -354,7 +359,7 @@ export default class Grid extends PureComponent {
   }
 
   componentDidMount () {
-    const { scrollLeft, scrollToColumn, scrollTop, scrollToRow } = this.props
+    const { getScrollbarSize, scrollLeft, scrollToColumn, scrollTop, scrollToRow } = this.props
 
     // If cell sizes have been invalidated (eg we are using CellMeasurer) then reset cached positions.
     // We must do this at the start of the method as we may calculate and update scroll position below.
@@ -500,6 +505,8 @@ export default class Grid extends PureComponent {
   }
 
   componentWillMount () {
+    const { getScrollbarSize } = this.props
+
     // If this component is being rendered server-side, getScrollbarSize() will return undefined.
     // We handle this case in componentDidMount()
     this._scrollbarSize = getScrollbarSize()
@@ -957,15 +964,17 @@ export default class Grid extends PureComponent {
   }
 
   _updateScrollLeftForScrollToColumn (props = this.props, state = this.state) {
-    const { columnCount, scrollToAlignment, scrollToColumn, width } = props
+    const { columnCount, height, scrollToAlignment, scrollToColumn, width } = props
     const { scrollLeft } = state
 
     if (scrollToColumn >= 0 && columnCount > 0) {
       const targetIndex = Math.max(0, Math.min(columnCount - 1, scrollToColumn))
+      const totalRowsHeight = this._rowSizeAndPositionManager.getTotalSize()
+      const scrollBarSize = totalRowsHeight > height ? this._scrollbarSize : 0
 
       const calculatedScrollLeft = this._columnSizeAndPositionManager.getUpdatedOffsetForIndex({
         align: scrollToAlignment,
-        containerSize: width,
+        containerSize: width - scrollBarSize,
         currentOffset: scrollLeft,
         targetIndex
       })
@@ -979,15 +988,17 @@ export default class Grid extends PureComponent {
   }
 
   _updateScrollTopForScrollToRow (props = this.props, state = this.state) {
-    const { height, rowCount, scrollToAlignment, scrollToRow } = props
+    const { height, rowCount, scrollToAlignment, scrollToRow, width } = props
     const { scrollTop } = state
 
     if (scrollToRow >= 0 && rowCount > 0) {
       const targetIndex = Math.max(0, Math.min(rowCount - 1, scrollToRow))
+      const totalColumnsWidth = this._columnSizeAndPositionManager.getTotalSize()
+      const scrollBarSize = totalColumnsWidth > width ? this._scrollbarSize : 0
 
       const calculatedScrollTop = this._rowSizeAndPositionManager.getUpdatedOffsetForIndex({
         align: scrollToAlignment,
-        containerSize: height,
+        containerSize: height - scrollBarSize,
         currentOffset: scrollTop,
         targetIndex
       })