Fix(Invetory): adding loading indicator into the inventory list #1991
Conversation
There was a problem hiding this comment.
@jeremielodi, this is a good first attempt. The component is exactly what the issue asked for! 👍
Unfortunately, this will never show - the loading indicator should turn on, wait for the data to load, and turn off. You can test this by setting the network tab in your chrome developer tools to us 'slow 3G' connections. The correct way to turn off the loading is shown above - either when the server has sent the data or an error.
Additionally, the handle error method should be renamed to make sure you don't break JS's error class.
| .catch(Notify.handleError); | ||
| .catch(Error); | ||
|
|
||
| vm.loading = false; |
There was a problem hiding this comment.
Does the loading indicator work? It seems like this will not wait for the data to download.
The correct way to wait for the data to download is to use the .finally() Promise method. Like this:
.catch(Notify.handleError)
.finally(function () {
vm.loading = false; // this will execute after the data is downloaded.
});| } | ||
|
|
||
| function Error() { |
There was a problem hiding this comment.
Error is a reserved word in JavaScript. Could you name this function something else?
| } | ||
|
|
||
| function Error() { | ||
| vm.hasError = true; | ||
| Notify.handleError(); |
There was a problem hiding this comment.
Notify.handleError() takes in a parameter - the error sent from the server. It should be passed to thus function.
|
It would also be good to put "fixes #" and reference the issue so that this closes the issue when merged.... |
|
@jeremielodi, let me know when you ready for another review. |
This PR helps to add a loading indicator to the inventory list