From 77ef864eda7cefb1f669bef4022a34574a916233 Mon Sep 17 00:00:00 2001 From: Steve Seear Date: Wed, 6 Jan 2016 12:40:43 +0000 Subject: [PATCH] Themes: replace infinite-list with infinite-scroll Use the infinite-scroll mixin for showcase scrolling, which suits showcase needs better than infinite-list. Benefits over Infinite-List when used in the showcase: * scroll performance improvement over master when scrolling up and once all themes are rendered (optimising the render may cure problems when rendering new themes) * InfiniteScroll is vastly less complex than InfiniteList * Unlike InfiniteList, InfiniteScroll should render on the server without modification * InfiniteScroll does not need the number of items per row to be calculated --- .../data/themes-list-fetcher/index.jsx | 3 -- shared/components/themes-list/Makefile | 2 +- shared/components/themes-list/index.jsx | 49 ++++++++++--------- shared/components/themes-list/test/index.jsx | 11 ++--- 4 files changed, 30 insertions(+), 35 deletions(-) diff --git a/shared/components/data/themes-list-fetcher/index.jsx b/shared/components/data/themes-list-fetcher/index.jsx index d75cdded03ef4..3b2cf4e59ff06 100644 --- a/shared/components/data/themes-list-fetcher/index.jsx +++ b/shared/components/data/themes-list-fetcher/index.jsx @@ -85,9 +85,6 @@ const ThemesListFetcher = React.createClass( { }, fetchNextPage: function( options ) { - // FIXME: While this function is passed on by `ThemesList` to `InfiniteList`, - // which has a `shouldLoadNextPage()` check (in scroll-helper.js), we can't - // rely on that; fetching would break without the following check. if ( this.props.loading || this.props.lastPage ) { return; } diff --git a/shared/components/themes-list/Makefile b/shared/components/themes-list/Makefile index b8f4179e84cc9..d045134e82079 100644 --- a/shared/components/themes-list/Makefile +++ b/shared/components/themes-list/Makefile @@ -4,7 +4,7 @@ COMPILERS ?= jsx:babel/register NODE_BIN := $(shell npm bin) MOCHA ?= $(NODE_BIN)/mocha BASE_DIR := $(NODE_BIN)/../.. -NODE_PATH := test:$(BASE_DIR)/shared +NODE_PATH := test:$(BASE_DIR)/shared:$(BASE_DIR)/client # In order to simply stub modules, add test to the NODE_PATH test: diff --git a/shared/components/themes-list/index.jsx b/shared/components/themes-list/index.jsx index bc02e81b47503..3c35f1b9b9083 100644 --- a/shared/components/themes-list/index.jsx +++ b/shared/components/themes-list/index.jsx @@ -2,14 +2,15 @@ * External dependencies */ var React = require( 'react' ), - times = require( 'lodash/utility/times' ); + times = require( 'lodash/utility/times' ), + isEqual = require( 'lodash/lang/isEqual' ); /** * Internal dependencies */ var Theme = require( 'components/theme' ), EmptyContent = require( 'components/empty-content' ), - InfiniteList = require( 'components/infinite-list' ), + InfiniteScroll = require( 'lib/mixins/infinite-scroll' ), ITEM_HEIGHT = require( 'lib/themes/constants' ).THEME_COMPONENT_HEIGHT, PER_PAGE = require( 'lib/themes/constants' ).PER_PAGE; @@ -17,9 +18,11 @@ var Theme = require( 'components/theme' ), * Component */ var ThemesList = React.createClass( { + + mixins: [ InfiniteScroll( 'fetchNextPage' ) ], + propTypes: { themes: React.PropTypes.array.isRequired, - lastPage: React.PropTypes.bool.isRequired, emptyContent: React.PropTypes.element, loading: React.PropTypes.bool.isRequired, fetchNextPage: React.PropTypes.func.isRequired, @@ -28,10 +31,13 @@ var ThemesList = React.createClass( { onMoreButtonClick: React.PropTypes.func, }, + fetchNextPage: function( options ) { + this.props.fetchNextPage( options ); + }, + getDefaultProps: function() { return { loading: false, - lastPage: false, themes: [], fetchNextPage: function() {}, optionsGenerator: function() { @@ -40,14 +46,14 @@ var ThemesList = React.createClass( { }; }, - getThemeRef: function( theme ) { - return 'theme-' + theme.id; + shouldComponentUpdate: function( nextProps ) { + return this.props.loading !== nextProps.loading || + ! isEqual( this.props.themes, nextProps.themes ); }, renderTheme: function( theme, index ) { - var key = this.getThemeRef( theme ); - return +
+ { themes } +
); } } ); diff --git a/shared/components/themes-list/test/index.jsx b/shared/components/themes-list/test/index.jsx index 611105bb8a363..f461abcf7f67b 100644 --- a/shared/components/themes-list/test/index.jsx +++ b/shared/components/themes-list/test/index.jsx @@ -17,7 +17,6 @@ function mockComponent( displayName ) { describe( 'ThemesList', function() { before( function() { mockery.registerMock( './more-button', mockComponent() ); - mockery.registerMock( 'components/infinite-list', mockComponent( 'InfiniteList' ) ); mockery.enable( { warnOnReplace: false, @@ -46,7 +45,8 @@ describe( 'ThemesList', function() { ], lastPage: true, loading: false, - fetchNextPage: function() {}, + fetchNextPage: () => {}, + getButtonOptions: () => {}, }; this.themesList = React.createElement( this.ThemesList, this.props ); @@ -66,16 +66,11 @@ describe( 'ThemesList', function() { this.themesListElement = shallowRenderer.getRenderOutput(); } ); - it( 'should render an InfiniteList with a className of "themes-list"', function() { + it( 'should render a div with a className of "themes-list"', function() { assert( this.themesListElement, 'element does not exist' ); - assert( this.themesListElement.type.displayName === 'InfiniteList', 'element type does not equal "InfiniteList"' ); assert( this.themesListElement.props.className === 'themes-list', 'className does not equal "themes-list"' ); } ); - it( 'should have an item for each theme', function() { - assert( this.themesListElement.props.items.length === this.props.themes.length, 'items count is different from themes count' ); - } ); - context( 'when no themes are found', function() { beforeEach( function() { var shallowRenderer = TestUtils.createRenderer();