tommoor / emojione-picker

A react emoji picker for use with emojione
http://tommoor.github.io/emojione-picker
MIT License
261 stars 61 forks source link

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

Closed ahutchings closed 7 years ago

ahutchings commented 7 years ago

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:

  1. 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.
  2. 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.

  1. 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.
  2. react-addons-shallow-compare needs to be installed as an app dependency.
tommoor commented 7 years ago

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 commented 7 years ago

Great idea @ahutchings.

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

ahutchings commented 7 years ago

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 commented 7 years ago

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.

tleunen commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

ahutchings commented 7 years ago

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

tommoor commented 7 years ago

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