Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Added scrollbar and more options in typeahead #595

Merged
merged 2 commits into from Jul 19, 2016
Merged

Added scrollbar and more options in typeahead #595

merged 2 commits into from Jul 19, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jul 17, 2016

Fixes #519 Medication/inventory typeahead issues

Changes proposed in this pull request:

  • Typeahead now displays a maximum of 10 options by adding limit in typeahead.js. Adding more than 10 appears to make the acceptance tests timeout
  • Typeahead now has a maximum height of 110px and displays a vertical scrollbar

Example

typeahead 2

cc @HospitalRun/core-maintainers

@jkleinsc
Copy link
Member

@cmwebby - thanks for the PR! Quick question - instead of limiting to 10 can we have it be unlimited with the scrollbar? Maybe 10 is good enough, but just curious what you found.

@ghost
Copy link
Author

ghost commented Jul 18, 2016

@jkleinsc Firstly can I apologise for making then closing a like a million pull requests! I don't normally work this way!
I think it certainly could be made to be unlimited but I think there may be a couple of issues with this:

  1. The main issue is whenever I tried adding a number more than 10 it made all the acceptance tests timeout. I couldn't spot the reason why it did this so I just stuck with 10.
    e.g

    not ok 74 PhantomJS 2.1 - Acceptance | medication: returning medication

    actual: >
        null
    message: >
        Test timed out
    Log: |
    
  2. If there are too many results say more than 50 it would probably just be quicker and easier for the user to just to type a few more characters in than searching all the possible results using the scroll bar.

If you have any ideas of why it would make the tests fail I am happy to go back and increae the limit.

@ghost
Copy link
Author

ghost commented Jul 18, 2016

@jkleinsc Ignore my previous comment for some reason the tests are passing now with more than 10. I have set a limit of 500, but it can be set to more if you want.

@jkleinsc
Copy link
Member

@cmwebby Looks good to me. I just tested it with a large dataset and the typeahead had no problems loading 500 suggestions, so I think we will stick with that. Thanks for the PR!

@jkleinsc jkleinsc merged commit ab737ff into HospitalRun:master Jul 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant