-
Notifications
You must be signed in to change notification settings - Fork 721
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
Add more elements until the container is filled #310
Conversation
@@ -131,7 +129,7 @@ angular.module(MODULE_NAME, []) | |||
return throttled; | |||
} | |||
|
|||
const handler = (THROTTLE_MILLISECONDS != null) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is handler reassigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true., I'll change back to const declaration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it needs to be let as it is resigned. es6lint doesn't allow for declaration before use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the should throttle or not logic into the throttle function, then we can just do:
const handler = throttle(unthrottledHandler, THROTTLE_MILLISECONDS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that defaultHandler calls handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, what should actually happen is something like:
async function doScroll() {
...
while(shouldScroll() && scrollEnabled) {
result = scope.infiniteScroll();
if (typeof result.then === 'function') {
await scope.infiniteScroll();
await $timeout(THROTTLE_MILLISECONDS);
} else {
await $timeout(THROTTLE_MILLISECONDS);
return;
}
}
...
}
function getHandler() {
let prom = null;
async function resetProm(doScrollProm) {
await doScrollProm;
prom = null;
}
async function innerHandler() {
if (!prom) {
prom = resetProm(doScroll());
}
await prom;
}
return innerHandler;
}
const handler = getHandler();
The idea, throttling is all handled by doScroll, every time it's called it loops until the container is full, while handling the throttling. However if infiniteScroll is a regular funciton with no promise, doScroll just defaults to the previous behaviour.
How to convert this from ES2017 async/await into $q then chains and support all the other features and checks I've left out is left as an exercise for the reader.
} else { | ||
if (checkInterval) { $interval.cancel(checkInterval); } | ||
checkWhenEnabled = false; | ||
$timeout(handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be recursive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not fully recursive as we have timeout and throttle in between but the idea is to check again after the digest is done.
Digest should make the dom change son that digest or trigger scrollEnabled = false if it is async calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fully recursive.... did you mean, not tail recursive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but the point here is that I do think we need a this call here
You don't know how long the infiniteScroll callback will take and a timeout poll is not really acceptable. This should only work if the infiniteScroll function returns a promise. |
I don't think it matters since disabled should be used to synchronize with async call right? |
@rleite ? |
If we can't fill the entire container on the first call the component will not fully filled until there is user interaction.
This is problematic if we have a small number of elements or if screen is really big.
This implementation will check if the container is fully filled after the digest of the handler call.