From 98c7ea733a969db78b7a5edb503ff330a7ebdcac Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sun, 3 Mar 2019 14:20:28 -0800 Subject: [PATCH] Deprecate list direction=horizontal|vertical in favor of direction=ltr|rtl and separate layout=horizontal|vertical --- src/FixedSizeList.js | 12 +- src/VariableSizeList.js | 12 +- src/__tests__/FixedSizeGrid.js | 8 +- src/__tests__/FixedSizeList.js | 73 +++++++++-- src/createListComponent.js | 115 ++++++++++++------ .../fixed-size-list-horizontal/index.js | 2 +- .../variable-size-list-horizontal/index.js | 2 +- website/src/code/FixedSizeListHorizontal.js | 2 +- .../src/code/FixedSizeListHorizontalRtl.js | 2 +- .../src/code/VariableSizeListHorizontal.js | 2 +- website/src/routes/api/FixedSizeList.js | 30 +++-- website/src/routes/examples/FixedSizeList.js | 2 +- website/src/routes/examples/RTLLayout.js | 1 - .../src/routes/examples/VariableSizeList.js | 2 +- 14 files changed, 187 insertions(+), 78 deletions(-) diff --git a/src/FixedSizeList.js b/src/FixedSizeList.js index 7491d91..498a8c9 100644 --- a/src/FixedSizeList.js +++ b/src/FixedSizeList.js @@ -15,12 +15,14 @@ const FixedSizeList = createListComponent({ ((itemSize: any): number) * itemCount, getOffsetForIndexAndAlignment: ( - { direction, height, itemCount, itemSize, width }: Props, + { direction, height, itemCount, itemSize, layout, width }: Props, index: number, align: ScrollToAlign, scrollOffset: number ): number => { - const size = (((direction === 'horizontal' ? width : height): any): number); + // TODO Deprecate direction "horizontal" + const isHorizontal = direction === 'horizontal' || layout === 'horizontal'; + const size = (((isHorizontal ? width : height): any): number); const maxOffset = Math.max( 0, Math.min( @@ -62,12 +64,14 @@ const FixedSizeList = createListComponent({ ), getStopIndexForStartIndex: ( - { direction, height, itemCount, itemSize, width }: Props, + { direction, height, itemCount, itemSize, layout, width }: Props, startIndex: number, scrollOffset: number ): number => { + // TODO Deprecate direction "horizontal" + const isHorizontal = direction === 'horizontal' || layout === 'horizontal'; const offset = startIndex * ((itemSize: any): number); - const size = (((direction === 'horizontal' ? width : height): any): number); + const size = (((isHorizontal ? width : height): any): number); return Math.max( 0, Math.min( diff --git a/src/VariableSizeList.js b/src/VariableSizeList.js index 75188c2..806beac 100644 --- a/src/VariableSizeList.js +++ b/src/VariableSizeList.js @@ -185,9 +185,11 @@ const VariableSizeList = createListComponent({ scrollOffset: number, instanceProps: InstanceProps ): number => { - const { direction, height, width } = props; + const { direction, height, layout, width } = props; - const size = (((direction === 'horizontal' ? width : height): any): number); + // TODO Deprecate direction "horizontal" + const isHorizontal = direction === 'horizontal' || layout === 'horizontal'; + const size = (((isHorizontal ? width : height): any): number); const itemMetadata = getItemMetadata(props, index, instanceProps); // Get estimated total size after ItemMetadata is computed, @@ -234,9 +236,11 @@ const VariableSizeList = createListComponent({ scrollOffset: number, instanceProps: InstanceProps ): number => { - const { direction, height, itemCount, width } = props; + const { direction, height, itemCount, layout, width } = props; - const size = (((direction === 'horizontal' ? width : height): any): number); + // TODO Deprecate direction "horizontal" + const isHorizontal = direction === 'horizontal' || layout === 'horizontal'; + const size = (((isHorizontal ? width : height): any): number); const itemMetadata = getItemMetadata(props, startIndex, instanceProps); const maxOffset = scrollOffset + size; diff --git a/src/__tests__/FixedSizeGrid.js b/src/__tests__/FixedSizeGrid.js index c55c8d9..ea6fd06 100644 --- a/src/__tests__/FixedSizeGrid.js +++ b/src/__tests__/FixedSizeGrid.js @@ -924,7 +924,7 @@ describe('FixedSizeGrid', () => { it('should fail if a string height is provided', () => { expect(() => ReactTestRenderer.create( - + ) ).toThrow( 'An invalid "height" prop has been specified. ' + @@ -936,11 +936,7 @@ describe('FixedSizeGrid', () => { it('should fail if a string width is provided', () => { expect(() => ReactTestRenderer.create( - + ) ).toThrow( 'An invalid "width" prop has been specified. ' + diff --git a/src/__tests__/FixedSizeList.js b/src/__tests__/FixedSizeList.js index d71b9f3..6e1eb1d 100644 --- a/src/__tests__/FixedSizeList.js +++ b/src/__tests__/FixedSizeList.js @@ -58,13 +58,32 @@ describe('FixedSizeList', () => { it('should render a list of columns', () => { ReactTestRenderer.create( - + ); expect(itemRenderer).toHaveBeenCalledTimes(5); expect(onItemsRendered.mock.calls).toMatchSnapshot(); }); + it('should re-render items if layout changes', () => { + const rendered = ReactTestRenderer.create( + + ); + expect(itemRenderer).toHaveBeenCalled(); + itemRenderer.mockClear(); + + // Re-rendering should not affect pure sCU children: + rendered.update(); + expect(itemRenderer).not.toHaveBeenCalled(); + + // Re-rendering with new layout should re-render children: + rendered.update(); + expect(itemRenderer).toHaveBeenCalled(); + }); + + // TODO Deprecate direction "horizontal" it('should re-render items if direction changes', () => { + spyOn(console, 'warn'); // Ingore legacy prop warning + const rendered = ReactTestRenderer.create( ); @@ -75,7 +94,7 @@ describe('FixedSizeList', () => { rendered.update(); expect(itemRenderer).not.toHaveBeenCalled(); - // Re-rendering with new direction should re-render children: + // Re-rendering with new layout should re-render children: rendered.update(); expect(itemRenderer).toHaveBeenCalled(); }); @@ -97,7 +116,7 @@ describe('FixedSizeList', () => { ReactDOM.render( , document.createElement('div') @@ -597,6 +616,32 @@ describe('FixedSizeList', () => { 'Please use the innerElementType and outerElementType props instead.' ); }); + + it('should warn if legacy direction "horizontal" value is used', () => { + spyOn(console, 'warn'); + ReactDOM.render( + , + document.createElement('div') + ); + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenLastCalledWith( + 'The direction prop should be either "ltr" (default) or "rtl". ' + + 'Please use the layout prop to specify "vertical" (default) or "horizontal" orientation.' + ); + }); + + it('should warn if legacy direction "vertical" value is used', () => { + spyOn(console, 'warn'); + ReactDOM.render( + , + document.createElement('div') + ); + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenLastCalledWith( + 'The direction prop should be either "ltr" (default) or "rtl". ' + + 'Please use the layout prop to specify "vertical" (default) or "horizontal" orientation.' + ); + }); }); describe('itemData', () => { @@ -664,6 +709,18 @@ describe('FixedSizeList', () => { ); }); + it('should fail if an invalid layout is provided', () => { + expect(() => + ReactTestRenderer.create( + + ) + ).toThrow( + 'An invalid "layout" prop has been specified. ' + + 'Value should be either "horizontal" or "vertical". ' + + '"null" was specified.' + ); + }); + it('should fail if an invalid direction is provided', () => { expect(() => ReactTestRenderer.create( @@ -671,7 +728,7 @@ describe('FixedSizeList', () => { ) ).toThrow( 'An invalid "direction" prop has been specified. ' + - 'Value should be either "horizontal" or "vertical". ' + + 'Value should be either "ltr" or "rtl". ' + '"null" was specified.' ); }); @@ -679,7 +736,7 @@ describe('FixedSizeList', () => { it('should fail if a string height is provided for a vertical list', () => { expect(() => ReactTestRenderer.create( - + ) ).toThrow( 'An invalid "height" prop has been specified. ' + @@ -691,11 +748,7 @@ describe('FixedSizeList', () => { it('should fail if a string width is provided for a horizontal list', () => { expect(() => ReactTestRenderer.create( - + ) ).toThrow( 'An invalid "width" prop has been specified. ' + diff --git a/src/createListComponent.js b/src/createListComponent.js index 8acb6e7..6db9cb8 100644 --- a/src/createListComponent.js +++ b/src/createListComponent.js @@ -9,7 +9,8 @@ import type { TimeoutID } from './timer'; export type ScrollToAlign = 'auto' | 'center' | 'start' | 'end'; type itemSize = number | ((index: number) => number); -type Direction = 'horizontal' | 'vertical'; +type Direction = 'ltr' | 'rtl' | 'horizontal' | 'vertical'; +type Layout = 'horizontal' | 'vertical'; type RenderComponentProps = {| data: T, @@ -49,6 +50,7 @@ export type Props = {| itemData: T, itemKey?: (index: number, data: T) => any, itemSize: itemSize, + layout: Layout, onItemsRendered?: onItemsRenderedCallback, onScroll?: onScrollCallback, outerRef?: any, @@ -130,8 +132,9 @@ export default function createListComponent({ _resetIsScrollingTimeoutId: TimeoutID | null = null; static defaultProps = { - direction: 'vertical', + direction: 'ltr', itemData: undefined, + layout: 'vertical', overscanCount: 2, useIsScrolling: false, }; @@ -151,15 +154,9 @@ export default function createListComponent({ // eslint-disable-next-line no-useless-constructor constructor(props: Props) { super(props); - } - static getDerivedStateFromProps( - props: Props, - state: State - ): $Shape | null { validateSharedProps(props); validateProps(props); - return null; } scrollTo(scrollOffset: number): void { @@ -188,10 +185,11 @@ export default function createListComponent({ } componentDidMount() { - const { initialScrollOffset, direction } = this.props; + const { direction, initialScrollOffset, layout } = this.props; if (typeof initialScrollOffset === 'number' && this._outerRef !== null) { - if (direction === 'horizontal') { + // TODO Deprecate direction "horizontal" + if (direction === 'horizontal' || layout === 'horizontal') { ((this ._outerRef: any): HTMLDivElement).scrollLeft = initialScrollOffset; } else { @@ -204,11 +202,12 @@ export default function createListComponent({ } componentDidUpdate() { - const { direction } = this.props; + const { direction, layout } = this.props; const { scrollOffset, scrollUpdateWasRequested } = this.state; if (scrollUpdateWasRequested && this._outerRef !== null) { - if (direction === 'horizontal') { + // TODO Deprecate direction "horizontal" + if (direction === 'horizontal' || layout === 'horizontal') { ((this._outerRef: any): HTMLDivElement).scrollLeft = scrollOffset; } else { ((this._outerRef: any): HTMLDivElement).scrollTop = scrollOffset; @@ -236,6 +235,7 @@ export default function createListComponent({ itemCount, itemData, itemKey = defaultItemKey, + layout, outerElementType, outerTagName, style, @@ -244,10 +244,13 @@ export default function createListComponent({ } = this.props; const { isScrolling } = this.state; - const onScroll = - direction === 'vertical' - ? this._onScrollVertical - : this._onScrollHorizontal; + // TODO Deprecate direction "horizontal" + const isHorizontal = + direction === 'horizontal' || layout === 'horizontal'; + + const onScroll = isHorizontal + ? this._onScrollHorizontal + : this._onScrollVertical; const [startIndex, stopIndex] = this._getRangeToRender(); @@ -286,6 +289,7 @@ export default function createListComponent({ overflow: 'auto', WebkitOverflowScrolling: 'touch', willChange: 'transform', + direction: direction === 'rtl' ? 'rtl' : 'ltr', ...style, }, }, @@ -293,9 +297,9 @@ export default function createListComponent({ children: items, ref: innerRef, style: { - height: direction === 'horizontal' ? '100%' : estimatedTotalSize, + height: isHorizontal ? '100%' : estimatedTotalSize, pointerEvents: isScrolling ? 'none' : '', - width: direction === 'horizontal' ? estimatedTotalSize : '100%', + width: isHorizontal ? estimatedTotalSize : '100%', }, }) ); @@ -379,10 +383,11 @@ export default function createListComponent({ // So that List can clear cached styles and force item re-render if necessary. _getItemStyle: (index: number) => Object; _getItemStyle = (index: number): Object => { - const { direction, itemSize } = this.props; + const { direction, itemSize, layout } = this.props; const itemStyleCache = this._getItemStyleCache( shouldResetStyleCacheOnItemSizeChange && itemSize, + shouldResetStyleCacheOnItemSizeChange && layout, shouldResetStyleCacheOnItemSizeChange && direction ); @@ -393,21 +398,25 @@ export default function createListComponent({ const offset = getItemOffset(this.props, index, this._instanceProps); const size = getItemSize(this.props, index, this._instanceProps); + // TODO Deprecate direction "horizontal" + const isHorizontal = + direction === 'horizontal' || layout === 'horizontal'; + itemStyleCache[index] = style = { position: 'absolute', - left: direction === 'horizontal' ? offset : 0, - right: direction === 'horizontal' ? offset : 0, - top: direction === 'vertical' ? offset : 0, - height: direction === 'vertical' ? size : '100%', - width: direction === 'horizontal' ? size : '100%', + left: isHorizontal ? offset : 0, + right: isHorizontal ? offset : 0, + top: !isHorizontal ? offset : 0, + height: !isHorizontal ? size : '100%', + width: isHorizontal ? size : '100%', }; } return style; }; - _getItemStyleCache: (_: any, __: any) => ItemStyleCache; - _getItemStyleCache = memoizeOne((_: any, __: any) => ({})); + _getItemStyleCache: (_: any, __: any, ___: any) => ItemStyleCache; + _getItemStyleCache = memoizeOne((_: any, __: any, ___: any) => ({})); _getRangeToRender(): [number, number, number, number] { const { itemCount, overscanCount } = this.props; @@ -458,15 +467,16 @@ export default function createListComponent({ return null; } - const isRtl = this.props.style && this.props.style.direction === 'rtl'; + const { direction } = this.props; return { isScrolling: true, scrollDirection: prevState.scrollOffset < scrollLeft ? 'forward' : 'backward', - scrollOffset: isRtl - ? scrollWidth - clientWidth - scrollLeft - : scrollLeft, + scrollOffset: + direction === 'rtl' + ? scrollWidth - clientWidth - scrollLeft + : scrollLeft, scrollUpdateWasRequested: false, }; }, this._resetIsScrollingDebounced); @@ -541,6 +551,7 @@ const validateSharedProps = ({ children, direction, height, + layout, innerTagName, outerTagName, width, @@ -553,12 +564,40 @@ const validateSharedProps = ({ ); } - if (direction !== 'horizontal' && direction !== 'vertical') { - throw Error( - 'An invalid "direction" prop has been specified. ' + - 'Value should be either "horizontal" or "vertical". ' + - `"${direction}" was specified.` - ); + // TODO Deprecate direction "horizontal" + const isHorizontal = direction === 'horizontal' || layout === 'horizontal'; + + switch (direction) { + case 'horizontal': + case 'vertical': + console.warn( + 'The direction prop should be either "ltr" (default) or "rtl". ' + + 'Please use the layout prop to specify "vertical" (default) or "horizontal" orientation.' + ); + break; + case 'ltr': + case 'rtl': + // Valid values + break; + default: + throw Error( + 'An invalid "direction" prop has been specified. ' + + 'Value should be either "ltr" or "rtl". ' + + `"${direction}" was specified.` + ); + } + + switch (layout) { + case 'horizontal': + case 'vertical': + // Valid values + break; + default: + throw Error( + 'An invalid "layout" prop has been specified. ' + + 'Value should be either "horizontal" or "vertical". ' + + `"${layout}" was specified.` + ); } if (children == null) { @@ -569,13 +608,13 @@ const validateSharedProps = ({ ); } - if (direction === 'horizontal' && typeof width !== 'number') { + if (isHorizontal && typeof width !== 'number') { throw Error( 'An invalid "width" prop has been specified. ' + 'Horizontal lists must specify a number for width. ' + `"${width === null ? 'null' : typeof width}" was specified.` ); - } else if (direction === 'vertical' && typeof height !== 'number') { + } else if (!isHorizontal && typeof height !== 'number') { throw Error( 'An invalid "height" prop has been specified. ' + 'Vertical lists must specify a number for height. ' + diff --git a/website/sandboxes/fixed-size-list-horizontal/index.js b/website/sandboxes/fixed-size-list-horizontal/index.js index bbf45a4..4c5ae27 100644 --- a/website/sandboxes/fixed-size-list-horizontal/index.js +++ b/website/sandboxes/fixed-size-list-horizontal/index.js @@ -13,10 +13,10 @@ const Column = ({ index, style }) => ( const Example = () => ( {Column} diff --git a/website/sandboxes/variable-size-list-horizontal/index.js b/website/sandboxes/variable-size-list-horizontal/index.js index 89f1d54..cc69d7d 100644 --- a/website/sandboxes/variable-size-list-horizontal/index.js +++ b/website/sandboxes/variable-size-list-horizontal/index.js @@ -21,10 +21,10 @@ const Column = ({ index, style }) => ( const Example = () => ( {Column} diff --git a/website/src/code/FixedSizeListHorizontal.js b/website/src/code/FixedSizeListHorizontal.js index 72cdefd..654196d 100644 --- a/website/src/code/FixedSizeListHorizontal.js +++ b/website/src/code/FixedSizeListHorizontal.js @@ -6,10 +6,10 @@ const Column = ({ index, style }) => ( const Example = () => ( {Column} diff --git a/website/src/code/FixedSizeListHorizontalRtl.js b/website/src/code/FixedSizeListHorizontalRtl.js index e1073aa..ad4f17e 100644 --- a/website/src/code/FixedSizeListHorizontalRtl.js +++ b/website/src/code/FixedSizeListHorizontalRtl.js @@ -6,7 +6,7 @@ const Column = ({ index, style }) => ( const Example = () => ( ( const Example = () => ( {Column} diff --git a/website/src/routes/api/FixedSizeList.js b/website/src/routes/api/FixedSizeList.js index dea1aed..2907d96 100644 --- a/website/src/routes/api/FixedSizeList.js +++ b/website/src/routes/api/FixedSizeList.js @@ -58,18 +58,14 @@ const PROPS = [ type: 'string', }, { - defaultValue: '"vertical"', + defaultValue: '"ltr"', description: ( -

Primary scroll direction of the list. Acceptable values are:

+

Determines the direction of text and horizontal scrolling.

    -
  • vertical (default) - Up/down scrolling.
    • -
  • horizontal - Left/right scrolling.
    • +
  • ltr (default)
    • +
  • rtl
-

- Note that lists may scroll in both directions (depending on CSS) but - content will only be windowed in the primary direction. -

), name: 'direction', @@ -207,6 +203,24 @@ const PROPS = [ name: 'itemSize', type: 'number', }, + { + defaultValue: '"vertical"', + description: ( + +

Layout/orientation of the list. Acceptable values are:

+
    +
  • vertical (default) - Up/down scrolling.
    • +
  • horizontal - Left/right scrolling.
    • +
+

+ Note that lists may scroll in both directions (depending on CSS) but + content will only be windowed in the layout direction specified. +

+ + ), + name: 'layout', + type: 'string', + }, { description: ( diff --git a/website/src/routes/examples/FixedSizeList.js b/website/src/routes/examples/FixedSizeList.js index e3cf340..f0ed7ec 100644 --- a/website/src/routes/examples/FixedSizeList.js +++ b/website/src/routes/examples/FixedSizeList.js @@ -54,11 +54,11 @@ export default function() { > {Item} diff --git a/website/src/routes/examples/RTLLayout.js b/website/src/routes/examples/RTLLayout.js index 4891b73..c07cf2f 100644 --- a/website/src/routes/examples/RTLLayout.js +++ b/website/src/routes/examples/RTLLayout.js @@ -39,7 +39,6 @@ export default function() { itemSize={100} layout="horizontal" width={300} - style={{ direction: 'rtl' }} > {Item} diff --git a/website/src/routes/examples/VariableSizeList.js b/website/src/routes/examples/VariableSizeList.js index 51aff13..c5e4b9e 100644 --- a/website/src/routes/examples/VariableSizeList.js +++ b/website/src/routes/examples/VariableSizeList.js @@ -58,11 +58,11 @@ export default function() { > columnSizes[index]} + layout="horizontal" width={300} > {Item}