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

[Enhancement] Add menuRenderer prop to support windowing for large lists #859

Merged
merged 20 commits into from
Apr 1, 2016

Conversation

bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Mar 31, 2016

I believe this PR resolves the following open issues: #554, #674, #739, #771, #807.

This PR adds support for windowing libraries like react-virtualized to be used to render large lists of options. It is similar to the approach mentioned in PR #763 but it avoids creating all of the menu options when rendering and so it performs better.

The size of the PR is mostly due to the fact that it contains a JSON data file of the largest 1,000 cities in Europe (used to demonstrate custom menu windowing). The actual changes to the Select component were very minimal (~40 new lines).

This change-set includes:

  • New renderMenu prop added to Select component.
  • New tests added to test/Select-test.js for new property. (Previous tests all pass.)
  • README updated to mention newly added property.
  • Example page updated to demonstrate new property as well. (Live demo of this functionality can be seen here, under "World's Largest Cities" header.

Note that react-virtualized was added to the devDependencies section (not dependencies) so it could be used in the example to demonstrate a more efficient way of rendering select menus with large amounts of data.

If this PR gets merged, or one like it, I plan to release a small adapter library with a single HOC that provides the react-virtualized drop-down menu automatically and passes through other props to Select. (In other words, I'm happy to take over the ownership of the feature.)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 03b3f00 on bvaughn:master into * on JedWatson:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 071ae88 on bvaughn:master into * on JedWatson:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 143bd64 on bvaughn:master into * on JedWatson:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9e11ce9 on bvaughn:master into * on JedWatson:master*.

@@ -250,6 +296,7 @@ For multi-select inputs, when providing a custom `filterOptions` method, remembe
optionRenderer | func | undefined | function which returns a custom way to render the options in the menu
options | array | undefined | array of options
placeholder | string\|node | 'Select ...' | field placeholder, displayed when there's no value
renderMenu | func | undefined | Renders a custom menu with options; accepts the following named parameters: `renderMenu({ focusedOption, focusOption, options, selectValue, valueArray })`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe menuRenderer would be better choice since it follows existing naming convention of optionRenderer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed. Thanks for the suggestion.

@cloudkite
Copy link
Contributor

Nice work! 👍

I think it would be good idea not to include examples/dist and lib folders in the PR, makes it alot more readable

@bvaughn
Copy link
Collaborator Author

bvaughn commented Mar 31, 2016

Truth. I wanted a live demo to point out so I had to push them to my fork. But maybe I'll clean up this PR and use another branch for the demo. Thanks for the input :)

@bvaughn bvaughn changed the title [Enhancement] Add renderMenu property for custom Menu rendering [Enhancement] Add menuRenderer prop to support windowing for large lists Mar 31, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3b591bf on bvaughn:master into * on JedWatson:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3b591bf on bvaughn:master into * on JedWatson:master*.

@bvaughn
Copy link
Collaborator Author

bvaughn commented Mar 31, 2016

Okay. Dist files have all been removed. Only large files still around are the example cities list.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9e18d92 on bvaughn:master into * on JedWatson:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a64c43f on bvaughn:master into * on JedWatson:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a64c43f on bvaughn:master into * on JedWatson:master*.

@JedWatson
Copy link
Owner

This looks fantastic @bvaughn! Thanks for the PR, and for owning the feature :)

@JedWatson JedWatson merged commit ddba4d7 into JedWatson:master Apr 1, 2016
@naveg
Copy link
Contributor

naveg commented Apr 2, 2016

This is great. It seems like it should be possible to use react-tether in the menuRenderer function, but I'm having trouble getting that to work. Problem is that I think the tether target should be the Select-control component, which isn't available here. Only thing I can think of it to add a dependency on react-tether and wrap the Select-control and menu in a TetherComponent, which we definitely don't want. Any ideas?

@Craga89
Copy link

Craga89 commented Apr 2, 2016

👍 to this. This is way more flexible than the menuComponent PR I made, great job @bvaughn! I'd love to see this merged so we can translate our current implementation over!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants