Typeahead popup position #3874
Typeahead popup position #3874
Conversation
scroll & resize events in 'append-to-body' version of render set popup position into right place
| // bind events only if appendToBody params exist - performance feature | ||
| if ( appendToBody ) { | ||
| angular.element( $window ).bind( 'resize', fireRecalculating ); | ||
| angular.element( document.body ).bind( 'scroll', fireRecalculating ); |
There was a problem hiding this comment.
These bindings should be throttled.
Also you can use $document.find('body') instead.
| var body = angular.element(document.body); | ||
|
|
||
| // Set body height to allow scrolling | ||
| body.css({height:"10000px"}); |
|
Thanks for comments, all done. Travis OK |
|
Any news for this enhancement? |
| // bind events only if appendToBody params exist - performance feature | ||
| if ( appendToBody ) { | ||
| angular.element( $window ).bind( 'resize', fireRecalculating ); | ||
| $document.find( 'body' ).bind( 'scroll', fireRecalculating ); |
There was a problem hiding this comment.
Both these event listeners need to be throttled - the resize and scroll events both fire often.
There was a problem hiding this comment.
Actually, a debounce on each of the event listeners would be better, since we only care about the trailing trigger of the event.
There was a problem hiding this comment.
Remove spaces just after ( and just before )
popup is hidden if scroll & resize is in progress
|
I added debounced execution of event logic and support for hiding popup if scroll & resize is "in progress". |
| @@ -170,6 +171,52 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap | |||
| }); | |||
| }; | |||
|
|
|||
| // bind events only if appendToBody params exist - performance feature | |||
| if ( appendToBody ) { | |||
There was a problem hiding this comment.
Remove spaces in between ( and )
|
|
||
| // Debounced executing recalculate after events fired | ||
| timeoutEventPromise = $timeout(function () { | ||
|
|
|
Done |
|
Adding a comment here for better visibility - there are multiple PRs involving positioning, so I am going to look into seeing if there is a better way of handling it inside $position instead. |
|
This PR looks safe to merge, although I think the debounce should be split off into a separate service - I'm inclined to hold off until this is done. I could merge this in now, or wait until the debounce is split off into a service - the benefit of holding off would be that debounce would be available as a utility injectable for anyone who needs it, but it does put some more burden on you. What are your thoughts? |
|
OK, the idea of separate debounce in service is good. But I think this should be in separate PR. I can write this after merge this PR. |
|
Only for my information: when are you planning to merge it into production? |
|
I think the debounce should be split off here - can you update your PR with that change? |
|
Actually, scratch my comment - I will merge this in today, and work can be done to split the debounce to a separate component after. |
Append-to-body dropdown position
Add support for typeahead-dropdown position after scroll and resize events.
Scenario 1
Scenario 2
Thanks