Code review comment for lp:~mzanetti/reminders-app/loading-property

Revision history for this message
David Planella (dpm) wrote :

Looks good to me, thanks to both Michael and Riccardo to get this one done. It's a huge improvement in terms of feedback to the user and the feel for responsiveness of the app.

Adding some context re: the discussion on when to show the activity indicator for the future.

<dpm> mzanetti, I think I agree with rpadovani that I'd hide the spinner on refreshes and perhaps just show it on the initial loading of the notes/notebooks lists (when they're empty). However, given the fact that the branch is a huge improvement already, and that the placement of the spinner might change with the new designs, I think we should go with your implementation, so top-approving, thanks!
<mzanetti> dpm: its close to no efforts to hide it when there's already something in the list
 dpm: if you want I can quickly do that before you top approve
<dpm> mzanetti, that'd mean that if I lose network connectivity once the list has been fetched, I won't get any spinner and thus no indication of flaky/no-network connection, correct?
<mzanetti> dpm: yep...
 dpm: again, I would keep it to inform the user when something is loading... but also not really in the center of the page
<mzanetti> dpm: lets keep it as is, gather some experience with it and ask Lucas to find a nice place where tho show it in the future
<dpm> mzanetti, ok, I agree, thanks!

review: Approve

« Back to merge proposal