Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Themes: replace infinite-list with infinite-scroll #2151

Merged
merged 1 commit into from
Jan 7, 2016

Conversation

seear
Copy link
Contributor

@seear seear commented Jan 6, 2016

Allow some comparisons between infinite-list and infinite-scroll with regards to complexity and performance.

No loading placeholders rendered yet.

Todo:

  • check for missing analytics events
  • no need to pass the lastPage prop into <ThemesList />

@seear seear self-assigned this Jan 6, 2016
@ehg ehg added this to the Themes: Showcase M3-LoggedOut milestone Jan 6, 2016
@seear seear changed the title [Experimental] Themes: replace infinite-list with infinite-scroll Themes: replace infinite-list with infinite-scroll Jan 7, 2016
@seear seear added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress [Type] Question labels Jan 7, 2016
@seear
Copy link
Contributor Author

seear commented Jan 7, 2016

This PR fires no fewer analytics events than master, though it seems the onLastPage event is not being fired on either branch.

@seear
Copy link
Contributor Author

seear commented Jan 7, 2016

Benefits of merging this PR to stop using InfiniteList.

  • scroll performance improvement over master when scrolling up and once all themes are rendered
    (optimising the <Theme /> render may cure problems when rendering new themes. See Themes: Get rid of some JSX prop .bind()s #2156)
  • 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 (see Themes: calc num columns for a smooth scroll #2103)

@seear seear force-pushed the try/themes-infinite-scroll branch from f5a2ce0 to 84dba29 Compare January 7, 2016 14:24
@ehg
Copy link
Member

ehg commented Jan 7, 2016

I think this works well. Tested on Chrome and Chrome mobile, the scrolling is much smoother. Code LGTM 👍

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 <Theme /> 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
@seear seear force-pushed the try/themes-infinite-scroll branch from f3e505c to 77ef864 Compare January 7, 2016 15:19
seear added a commit that referenced this pull request Jan 7, 2016
Themes: replace infinite-list with infinite-scroll
@seear seear merged commit 20def27 into master Jan 7, 2016
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 7, 2016
@seear seear deleted the try/themes-infinite-scroll branch January 7, 2016 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants