Virtualize the emoji category listing to avoid performance hit on initial render #50

ahutchings · 2017-02-01T00:26:32Z

This isn't ready for a merge just yet, but I wanted to open this pull request to get some feedback on the direction and integrate any changes you'd like to make.

These changes address #45, which describes the momentary lag that you get when first opening the emoji picker. This lag is caused by two things:

First, if you are not using a sprite sheet, then an HTTP request is made for each rendered emoji. The requests are bottlenecked by the browser's per-origin concurrent request limits. We can update the README to more strongly recommended the sprite sheet approach and perhaps pull in some more examples of how to use the emojione-provided SVG/PNG sprite sheets.
All available emoji (~1800) are rendered into the DOM in one pass, taking up to 2-3 seconds to complete on older machines. This pull request addresses this issue by using react-virtualized to only render the emoji that are needed based on scroll position. This ensures that only one to three categories are rendered at any given time.

One caveat of this from a code maintenance perspective is that all of the styles that affect the category layout need to be inline or pulled in via some other means of style modularization. react-virtualized's CellMeasurer component uses a trick of pre-rendering categories elsewhere in the DOM in order to measure their size, effectively breaking their inheritance-based styling that was applied via the CSS file. The react-virtualized docs go into a little more detail describing the how and why. I've worked around this for now by putting all of the category styles inline, leaving the rest of the styles in the CSS file. However, I think this situation should be addressed with a more cohesive solution before merging.

Update: I've pushed an update that no longer uses CellMeasurer, which gives a small perf improvement and also means that all styles can continue to be defined in the stylesheet instead of inline. There is a small amount of duplication here between the stylesheet and the component that calculates the category height - this will need to be kept in sync. If any category-layout-affecting style properties are overridden by the application this would throw react-virtualized's positioning off, so this is probably worth documenting unless there is some other workaround.

There are a couple additional steps that dependent applications will need to take to use this new version. I will update the README with this information once we've answered some of the questions about how to package and distribute the new version.

react-virtualized's CSS needs to be loaded by the app. This is located in react-virtualized's package at react-virtualized/styles.css. We could inline these styles into css/picker.css, but I'd be concerned about the styles getting out of sync. The react-virtualized-select project recommends loading the CSS files separately. Do you have any thoughts around this? Update: The stylesheet will not be needed, see my later comment.
react-addons-shallow-compare needs to be installed as an app dependency.

…tial render Refs #45

This eliminates the need for CellMeasurer, which slightly improves performance and allows us to move styling back to the CSS file.

…e shape of the return value

This eliminates the need for CellMeasurer, which slightly improves performance and allows us to move styling back to the CSS file.

tommoor · 2017-02-01T16:42:46Z

Hey @ahutchings - just a quick note to say that these changes look like they're definitely in a positive direction, I'll try and give them a thorough looking over before the end of the week.

lochstar · 2017-02-01T21:42:04Z

Great idea @ahutchings.

Just a note that react-addons-shallow-compare is deprecated. You can use PureComponent instead.

ahutchings · 2017-02-01T23:47:11Z

PureComponent was introduced in react v15.3.0, so we can't use it just yet unless we want to drop v14 support from emoji picker.

However, we'll still have a transitive dependency on shallow-compare through react-virtualized until they get a chance to upgrade. The react-virtualized maintainer has said that he will look at upgrading to PureComponent once react v16 drops.

ahutchings · 2017-02-02T20:29:14Z

I just discovered that the react-virtualized styles that we depend on are all inline, so emojione-picker users will not need to include react-virtualized's stylesheet. I'll push a new commit shortly to remove the link to it from the preview html.

…e HTML page

tleunen · 2017-02-03T14:55:25Z

Great idea @ahutchings. If this lands in master, I'll definitely try it again :)

Imo, it's always great to support the last 2 versions of React. So keeping React 14 for now might be best, even though I believe most users now use React 15.

If React 16 lands before this PR, maybe you'll be able to also drop react 14 support here :)

ahutchings · 2017-02-08T15:40:23Z

This pull request is in pretty good shape now and should be safe to review. It's been working well in our testing, so I don't intend to push any more commits other than in response to feedback.

tommoor

Overall these changes look great! I've pointed out a couple of quick improves in the code and I also noticed that the emojis and skin tone picker are not aligned correctly on the right with the search box…

tommoor · 2017-02-11T22:42:40Z

src/utils/createEmojisFromStrategy.js

@@ -0,0 +1,30 @@
+export default function createEmojisFromStrategy(strategy) {


Nice, thanks for moving this out of the component 💯

tommoor · 2017-02-11T22:45:43Z

src/utils/createRowsSelector.js

@@ -0,0 +1,71 @@
+import {chunk, map, values} from 'lodash';


Could you import lodash methods individually to reduce bundle size? I.e in the form:

import chunk from 'lodash/chunk';

tommoor · 2017-02-11T22:50:00Z

src/categories.jsx

@@ -0,0 +1,163 @@
+import React, {PropTypes, Component} from 'react';
+import {AutoSizer, List} from 'react-virtualized';
+import {findIndex, throttle} from 'lodash';


Same comment re: lodash imports

tommoor · 2017-02-11T22:50:52Z

src/categories.jsx

+import EmojiRow from './emoji-row';
+import Modifiers from './modifiers';
+
+const CATEGORY_HEADER_ROW_HEIGHT = 46;


Lets add a note here that these need keeping in sync with the CSS file

tommoor · 2017-02-11T22:53:21Z

src/emoji-row.jsx

+              role="option"
+              key={emoji.unicode}
+              onClick={function(e) {
+                onChange(emoji);


We're defining functions in a loop here within the render, so this is not going to be performant. Looking back I see it was like this before so I could go ahead and address this from a PR after this lands if you prefer 😄

How would you prefer to do this? The change handlers need to be bound to to the emoji object being selected, so I believe we'll at least need to allocate at least one function per emoji.

One possibility -
The onClick and onKeyUp handlers are both intended to select the emoji, so we could define the onClick and onKeyUp handlers in the Emoji component instead, and the component rendering the Emoji component could pass a single onSelect prop instead. That should save one function allocation per emoji, since the onClick and onKeyUp functions would be shared on the Emoji prototype. It would also have the benefit of consolidating the keyup key filtering logic, because the Emoji component is rendered by two different components.

One way would be to only pass the onChange callback to Emoji. And inside emoji do what you need to do, while binding the function only once in the constructor. (Same thing for the keyUp).

tommoor · 2017-02-11T22:56:38Z

It's funny how many problems I can spot in my own code from just 6 months ago… Thankfully you tackled many of them 😆

…istent with emoji rows

ahutchings · 2017-02-12T02:25:45Z

Most of the review comments are addressed now - just had one question about the direction you want to go with src/emoji-row.jsx.

Move the lower-level onClick and onKeyUp handlers down into the Emoji component, and expose a single onSelect prop from Emoji.

tleunen · 2017-02-12T16:15:38Z

src/emoji-row.jsx

-                if (e.which === 13 || e.which === 32) {
-                  onChange(emoji);
-                }
+              onSelect={() => {


You can also do onSelect={this.props.onChange} and keep the Emoji component the responsability to send the right information.

The difference will be that instead of creating a function for each emoji for each render, you'll be creating one function for each emoji only.

The Emoji component is also used in the category list in the header, where the parent component needs the category id instead of the emoji object when the Emoji component is selected. I decided to keep the contract as is since it doesn't assume anything about what the parent component will do when the Emoji is selected. Performance is still good since updates are prevented at the row level.

tommoor · 2017-02-12T22:20:22Z

I'm working on putting together a 2.0.0 release with these changes and some other minor fixes, thanks for all your work! 🙌

ahutchings added 10 commits January 30, 2017 11:15

Add npm-debug.log to .gitignore

c4b4be9

Virtualize the emoji category listing to avoid performance hit on ini…

31b4987

…tial render Refs #45

Make category layout properties JavaScript constants

51998f1

This eliminates the need for CellMeasurer, which slightly improves performance and allows us to move styling back to the CSS file.

Reorder CSS to be closer to the base branch

b6f7d16

Reorder CSS to be closer to the base branch

eb7618a

Rename createCategorySelector to createCategoriesSelector to match th…

dcffa19

…e shape of the return value

Remove unnecessary ref

84f89a6

Make category layout properties JavaScript constants

a96cb82

This eliminates the need for CellMeasurer, which slightly improves performance and allows us to move styling back to the CSS file.

Remove unused method

e8b8d60

Fix left and right padding of the emoji category toolbar

4940b09

Ignore tests when compiling source files

92b4c64

ahutchings mentioned this pull request Feb 2, 2017

Search doesn't work as expected #43

Closed

ahutchings added 3 commits February 2, 2017 17:30

Remove unnecessary link to react-virtualized's styles from the exampl…

1751d10

…e HTML page

Virtualize emoji categories at the row level for better performance

00f47af

Fix emoji width and row padding

0b2ef84

tommoor suggested changes Feb 11, 2017

View reviewed changes

ahutchings added 3 commits February 11, 2017 18:55

Cherry-pick lodash methods for smaller bundle sizes

1a8a3fc

Add a note to keep the virtualized row height constants in sync with CSS

4df23cc

Make horizontal padding of the search input and category headers cons…

128e671

…istent with emoji rows

Reduce number of functions created when rendering an Emoji

5371cda

Move the lower-level onClick and onKeyUp handlers down into the Emoji component, and expose a single onSelect prop from Emoji.

tleunen reviewed Feb 12, 2017

View reviewed changes

tommoor merged commit 4a22b9c into tommoor:master Feb 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Virtualize the emoji category listing to avoid performance hit on initial render #50

Virtualize the emoji category listing to avoid performance hit on initial render #50

ahutchings commented Feb 1, 2017 •

edited

Loading

tommoor commented Feb 1, 2017

lochstar commented Feb 1, 2017

ahutchings commented Feb 1, 2017 •

edited

Loading

ahutchings commented Feb 2, 2017

tleunen commented Feb 3, 2017

ahutchings commented Feb 8, 2017

tommoor left a comment

tommoor Feb 11, 2017

tommoor Feb 11, 2017

ahutchings Feb 12, 2017

tommoor Feb 11, 2017

ahutchings Feb 12, 2017

tommoor Feb 11, 2017

ahutchings Feb 12, 2017

tommoor Feb 11, 2017

ahutchings Feb 12, 2017

tleunen Feb 12, 2017

tommoor commented Feb 11, 2017 •

edited

Loading

ahutchings commented Feb 12, 2017

tleunen Feb 12, 2017

ahutchings Feb 12, 2017

tommoor commented Feb 12, 2017

		@@ -0,0 +1,30 @@
		export default function createEmojisFromStrategy(strategy) {

Virtualize the emoji category listing to avoid performance hit on initial render #50

Virtualize the emoji category listing to avoid performance hit on initial render #50

Conversation

ahutchings commented Feb 1, 2017 • edited Loading

tommoor commented Feb 1, 2017

lochstar commented Feb 1, 2017

ahutchings commented Feb 1, 2017 • edited Loading

ahutchings commented Feb 2, 2017

tleunen commented Feb 3, 2017

ahutchings commented Feb 8, 2017

tommoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommoor commented Feb 11, 2017 • edited Loading

ahutchings commented Feb 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommoor commented Feb 12, 2017

ahutchings commented Feb 1, 2017 •

edited

Loading

ahutchings commented Feb 1, 2017 •

edited

Loading

tommoor commented Feb 11, 2017 •

edited

Loading