chieffancypants / angular-loading-bar

A fully automatic loading / progress bar for your angular apps.
https://chieffancypants.github.io/angular-loading-bar/
MIT License
5.17k stars 682 forks source link

Feature/allow manual control #357

Open ArlyMathiasen opened 7 years ago

ArlyMathiasen commented 7 years ago

@faceleg @mattbrunetti This addresses the same problem as #49 in a more flexible way.

adds push and pop methods, to allow for manually simulating the initialization and completion of requests.

These functions were exposed on the service API, allowing developers using this package to set up custom triggers for setting the progress of the bar / completing the bar, using whatever 3rd party libraries they want.

I've updated the dev dependancies and made changes to code where those updates cause breaking changes.

please review

mattbrunetti commented 7 years ago

@ArlyMathiasen whitespace changes make the "add push and pop methods …" commit look like a complete rewrite... also would create merge conflicts with any PR that touches that file...

mattbrunetti commented 7 years ago

@ArlyMathiasen In regards to the README update

It is suggested that you turn off autoIncrement if you plan on using this

Why? The intended purpose of that feature is so that, for example, during the time we have 1 outstanding request, the user will see the bar moving, indicating "something is happening". I believe otherwise it would sit at 1% until the request completes.

ArlyMathiasen commented 7 years ago

@mattbrunetti sorry about that, my editor auto-indented some of it, fixed now.

ArlyMathiasen commented 7 years ago

@mattbrunetti Because of how the code was defined before (note that push and pop were refactored out of where actual requests were being handled) calling pop invokes _set(reqsCompleted / reqsTotal) when it's not the last pop needed, if autoIncrement is on this can result in sometimes the bar going backwards if some instance of pop takes a large enough time to be called.

ArlyMathiasen commented 7 years ago

@mattbrunetti If you think that's unlikely enough I can take that out of the README

mattbrunetti commented 7 years ago

@ArlyMathiasen This is also possible with the automatic triggering based on $http requests, so nothing specific about manual triggering, and that's the way it was before. I believe the default of having it enabled is generally preferred; It's not perfect but gives the clear impression that "something's happening" and should not appear funky most of the time. I would not advise against the default behavior.

Also, maybe keep the description of the methods short and high-level; no need to go into such detail about implementation details. Less is more. e.g. "Signals the start of a request". Should still mention the optional argument(s). Also good to note when you would need it - "Use this if your request doesn't go through the $http service and thus can't be automatically detected."

Looks good otherwise

ArlyMathiasen commented 7 years ago

@mattbrunetti changed it to

// If your requests don't go through the $http service and can't be automatically detected, // use the following cfpLoadingBar.push(x) // signals the start of a request // it will broadcast x on the 'cfpLoadingBar:loading' event when called cfpLoadingBar.pop(y) // signals the completion of a request // if it is the last request to complete, broadcasts y on the 'cfpLoadingBar:loaded' event