Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(interim, toast): add hide queue to prevent multiple interims at same time #6684

Closed
wants to merge 1 commit into from

Conversation

devversion
Copy link
Member

@devversion devversion commented Jan 14, 2016

After this patch, the interim stack will pick up the first added element from the stack which
is not in hide process. At the moment we always take the first added (not caring about current hide process).

Also the stack always removes the elements immediately, which causes errors when showing multiple interims (for example spamming toasts). After this fix, we will remove the elements after the hide-process is done (like a stack queue)

Fixes #6633 Fixes #8111 Fixes #4822

@@ -291,6 +291,12 @@ function InterimElementProvider() {
// This hide()s only the current interim element before showing the next, new one
// NOTE: this is not reversible (e.g. interim elements are not stackable)

if (!hideExisting) {
return $q(function(resolve, reject) {
reject("Too many interim elements in queue!");
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the rejection, but that's the best way to prevent the interim element to stuck on showing. Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

return $q.reject('Too many interim elements in queue!');

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, definitely better!

@GongT
Copy link

GongT commented Jan 30, 2016

cant get your point
But var hideExisting = !options.skipHide && stack.length ? service.hide() : $q.when(true); will constantly True. you must misunderstand stack.

@devversion
Copy link
Member Author

@GongT - I think you just didn't read the complete code. I will talk through the steps with you...
If we show an interim element, we check if have already an element in the stack, if so we close the previous element in stack, calling the hide method, their we will return the stack element (but not remove that instantly as before), so the stack will work as desired. In the current version we always removed the interim element instantly from the stack and returned the instance, that's why multiple interim elements will be removed in a wrong order.

In this PR, we only remove the element from the stack if the hide is completed.
If we destroy the interim element through the API, we remove the latest element in stack using our queue count, so the order will be always right!

@ThomasBurleson ThomasBurleson added the needs: review This PR is waiting on review from the team label Jan 30, 2016
@ThomasBurleson ThomasBurleson added this to the 1.1.0 milestone Jan 30, 2016
@ThomasBurleson ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Jan 30, 2016
var promise = $q.all(stack.reverse().map(closeElement));
stack = [];
return promise;
return $q.all(stack.reverse().map(closeElement));
} else if (options.closeTo !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

can you fix it to be just options.closeTo

also the else statement is redundant because we return value from the last if statement

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, but I don't get what you mean with the else statement?

if (options.closeAll) {
  return $q.all(stack.reverse().map(closeElement));
} else if (options.closeTo) {
  return $q.all(stack.slice(options.closeTo).map(closeElement));
} else {
  return closeElement(stack[hideQueue]);
}

Every check will be used. Mostly this return closeElement(stack[hideQueue]); when showing / hiding a toast

Copy link
Member

Choose a reason for hiding this comment

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

what i meant is some thing like this:

if (options.closeAll) {
  return $q.all(stack.reverse().map(closeElement));
} 

if (options.closeTo) {
  return $q.all(stack.slice(options.closeTo).map(closeElement));
}

return closeElement(stack[hideQueue]);

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh got it 😄 Done!

@devversion devversion force-pushed the fix/interim-hide-queue branch 6 times, most recently from 8c151f3 to 99d90ab Compare February 7, 2016 15:42
After this patch, the interim stack will pick up the first added element from the stack which
is not in hide process. At the moment we always take the first added (not caring about current hide process).

Also the stack always removes the elements immediately, which causes errors when showing multiple interims (for example spamming toasts). After this fix, we will remove the elements after the hide-process is done (like a stack queue)

Fixes angular#6633 Fixes angular#4822
@devversion devversion force-pushed the fix/interim-hide-queue branch from 99d90ab to 6ce97aa Compare February 7, 2016 15:43
@devversion
Copy link
Member Author

@EladBezalel - Updated the description of the PR (also waiting for #7053 for being merged). Just made this PR compatible with the new changes from #7053.

After this patch, the interim stack will pick up the first added element from the stack which
is not in hide process. At the moment we always take the first added (not caring about current hide process).

Also the stack always removes the elements immediately, which causes errors when showing multiple interims (for example spamming toasts). After this fix, we will remove the elements after the hide-process is done (like a stack queue)

@devversion
Copy link
Member Author

Closing this, because I will improve that, when we starting to support multiple dialogs.

That means, we need to take a more accurate look at the interim element stack.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: review This PR is waiting on review from the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"TypeError: Cannot read property 'focus' of undefined" when using md-tabs mdToast removed early Toasts overlap
4 participants