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

Used virtualized scrolling for AutoComplete component #568

Merged
merged 1 commit into from
Jun 23, 2016

Conversation

noisecapella
Copy link
Contributor

@noisecapella noisecapella commented Jun 17, 2016

What are the relevant tickets?

Fixes #515

What's this PR do?

Adds Menu and List from material-ui and modifies them and AutoComplete to increase performance and use virtualized scrolling.

Where should the reviewer start?

How should this be manually tested?

In general things should act exactly as they did before, except faster hopefully or at least not slower. Open up micromasters-ci in a separate tab and for each case compare functionality between it and your local instance which is on this branch.

First, go through all of the cases in the manual testing section of #382 and verify that it all works. Additionally check these cases (make sure to confirm the redux state for each selection):

  • You should be able to press down to select the first item in the menu. If you press enter you should select that item in the list.
  • Type text to filter the results, then press down and select some other item in the list. If you press enter you should select that item in the list.
  • Type text to filter the results, press down, then click an item on the list. It should be selected successfully.
  • Type text then press enter. The first item in the results should be selected.

Other things to check for:

  • scrollbars should show up if and only if appropriate. You should be able to use them like any other list.
  • When you click on a autocomplete field with many items, like the country field, it should open about as quickly as any other autocomplete field on the page. It should not be slower than it was before.
  • You should be able to scroll to the bottom of the list and select the last item without any problem.
  • The popup height should be at most 300 pixels, and if less space is needed it should shrink to fit the items in the list.
  • The width should match the text field's width.

Bugs:

  • if you press the page down key a few times and then press down, it will continue scrolling. This is different from the previous behavior where the down key would move to the next selected item in the list, no matter how far down in the list you go.

What GIF best describes this PR or how it makes you feel?

http://i.giphy.com/zf3skxc4BFBqo.gif

@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 15:58 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 16:12 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 16:20 Inactive
@noisecapella noisecapella changed the title Gs/experiment Used virtualized scrolling for AutoComplete component Jun 17, 2016
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 16:22 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 16:24 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 16:27 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 16:33 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 17:55 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 18:00 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 17, 2016 18:25 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 20, 2016 15:03 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 20, 2016 15:12 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 20, 2016 15:17 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 20, 2016 15:23 Inactive
@gsidebo gsidebo self-assigned this Jun 20, 2016
@@ -393,7 +405,8 @@ class AutoComplete extends Component {
triggerUpdateOnFocus, // eslint-disable-line no-unused-vars
openOnFocus, // eslint-disable-line no-unused-vars
maxSearchResults,
dataSource,
dataSource, // eslint-disable-line no-unused-vars
menuHeight,
Copy link
Contributor

Choose a reason for hiding this comment

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

seems odd that menuHeight wouldn't be part of menuStyle. is there a reason for this? either way, this should be moved up to be proximal to menuStyle and menuProps in this list

Copy link
Contributor Author

@noisecapella noisecapella Jun 20, 2016

Choose a reason for hiding this comment

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

I added menuHeight because we need to pass this number to the VirtualScroll component

Copy link
Contributor

@gsidebo gsidebo Jun 21, 2016

Choose a reason for hiding this comment

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

that's a concern of the child component. i still think height should be part of menuStyle (as 'maxHeight'). Menu.js can worry about passing the correct value for 'menuHeight'. this would get rid of the need for 'menuStyleWithHeight', which IMO is kind of clunky

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it back to menuStyle but we still need to pass it as a separate prop since maxHeight is not the same thing as VirtualScroll's height, and it seems clunky to extract that information from a CSS style object

@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 20, 2016 16:21 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 20, 2016 16:35 Inactive
@gsidebo
Copy link
Contributor

gsidebo commented Jun 20, 2016

just checked out the branch and ran through the profile forms. this is performing really nicely. should we think about manually testing this in other browsers?

@noisecapella
Copy link
Contributor Author

Yes, we generally support the same browsers as edX: http://edx.readthedocs.io/projects/edx-guide-for-students/en/latest/front_matter/browsers.html

@gsidebo
Copy link
Contributor

gsidebo commented Jun 21, 2016

ss 2016-06-21 at 12 14 49 pm

trying to test in an Win7/IE11 VM. happening on -ci too. ill file an issue for this. as it stands, i can't test this in IE11, soooo

👍

@noisecapella
Copy link
Contributor Author

I filed an issue to fix this: #609

@gsidebo
Copy link
Contributor

gsidebo commented Jun 21, 2016

hold on a sec - jumped the gun on the +1. i just want to review the changes you made once more

@noisecapella
Copy link
Contributor Author

That's fine, we have the merge freeze anyway

@gsidebo
Copy link
Contributor

gsidebo commented Jun 21, 2016

works in IE11

EDIT: that might not be true since the polyfill hasnt been added to this PR. i may have been working with the wrong static assets

@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 21, 2016 20:46 Inactive
@noisecapella
Copy link
Contributor Author

I rebased on master so this PR should now include the polyfill

@bdero bdero temporarily deployed to micromasters-ci-pr-568 June 22, 2016 15:27 Inactive
@noisecapella
Copy link
Contributor Author

Any other comments?

@gsidebo
Copy link
Contributor

gsidebo commented Jun 23, 2016

tested in IE11. looks great! 👍

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

Successfully merging this pull request may close these issues.

3 participants