Cristian Rodriguez (:crdlc) Assignee
	Description • 10 years ago

      No description provided.

	Cristian Rodriguez (:crdlc) Assignee
	Comment 1 • 10 years ago

Status: tt is already implemented and right now I am working on unit tests

	Cristian Rodriguez (:crdlc) Assignee
	Comment 2 • 10 years ago

Hi Michael,

   Vivien told me that you are the best :) to review this patch!

Thanks for your time mate

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 3 • 10 years ago

Hi Crisian! Thank you for the patch (and especially the unit tests)! I am working on some other bugs at the moment, so I might not get to this today, but it's on my radar :)

	Cristian Rodriguez (:crdlc) Assignee
	Comment 4 • 10 years ago

Thanks a lot for your help Michael

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 5 • 10 years ago

Looking good Cristian! Thank you especially for adding unit tests! I left some comments on the PR.

Also, I think we will need a quick review from UX on these changes. I'm not sure if all the scenarios were considered with respect to hiding the retry button and tap to retry message. Can you upload some screenshots of both the framed and non-framed screens for netOffline, and flag :djabber for UX review?

	Cristian Rodriguez (:crdlc) Assignee
	Comment 6 • 10 years ago

Hi Michael,

   Thanks for the review, I gonna address all your comments today

(In reply to Michael Henretty [:mhenretty] from comment #5)
> Looking good Cristian! Thank you especially for adding unit tests! I left
> some comments on the PR.
> 
> Also, I think we will need a quick review from UX on these changes. I'm not
> sure if all the scenarios were considered with respect to hiding the retry
> button and tap to retry message. Can you upload some screenshots of both the
> framed and non-framed screens for netOffline, and flag :djabber for UX
> review?

I am hiding the retry button just for net errors because this patch implements the automatic reload of apps when the device is online. If the user is seeing the network error is because of the app cannot be reloaded.

Francis, do you think what there are some scenario where the retry-button makes sense for network error?. If the error is for DNS, server, etc.. there is a retry button for sure like before.

I will upload some pics of this change today too

Thanks mates

	Cristian Rodriguez (:crdlc) Assignee
	Comment 7 • 10 years ago

No retry button

	Cristian Rodriguez (:crdlc) Assignee
	Comment 8 • 10 years ago

No retry button for apps

	Cristian Rodriguez (:crdlc) Assignee
	Comment 9 • 10 years ago

Comments addressed! Thx

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 10 • 10 years ago

Thanks again Cristian! I'll take a look at this next week.

	Cristian Rodriguez (:crdlc) Assignee
	Comment 11 • 10 years ago

Please Michael, take a look to this one in order to continue working on bug 969267 :) I think that it is fast because it is just a second review after addressing your comments. Thanks a lot

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 12 • 10 years ago

Comment on attachment 8370083 [details]
Patch v1

A couple more comments/questions on the PR. Other than that this is looking good. Go ahead and re-flag me when it's ready, or ping me on IRC if you have questions.

	Cristian Rodriguez (:crdlc) Assignee
	Comment 13 • 10 years ago

Comment on attachment 8370083 [details]
Patch v1

Thanks, addressed comments.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 14 • 10 years ago

Comment on attachment 8370083 [details]
Patch v1

Looks like you have a debug commit in that PR, so I added a couple of comments to the first commit. They are two very small nits, other than that this looks awesome. Thanks!

We still need UX signoff, and I am waiting to hear back from :pdol about whether we can land this independently of the other offline stuff. My guess is yes, but we'll wait on them. Nice work!

Comments here.
https://github.com/crdlc/gaia/commit/a3ff11fc1d8505a3417efc50794c3185786e8256

	Francis Djabri [:djabber]
	Comment 15 • 10 years ago

Comment on attachment 8371385 [details]
framed offline.png

I agree that it doesn't make any sense to show this in-frame error without the option of tapping to retry. Please keep the tap-to-retry button in for this case.

	Francis Djabri [:djabber]
	Comment 16 • 10 years ago

Comment on attachment 8371386 [details]
unframed offline.png

I also agree that it makes no sense to show this dialog without a retry button. However, if it is a network error that is the problem and not the device, we should alter the message, because this gives the user the impression that there is an issue with the device and then gives them no means to fix it. This should be fixed by Bug #912445.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 17 • 10 years ago

After speaking with Francis over vidyo now, we decided the issue with unframed offline screen is not hiding the retry button (we need to do this), but that it doesn't make sense to have this screen unless we provide resolution actions (ie going to settings an enabling wifi, or something). So we are going to have to block landing this bug on that unfortunately.

As far as the framed case, even though the retry button doesn't in a sense do anything (since the frame will automatically refresh), he still thinks its nicer to the user to give him the ability to manually try.

	Cristian Rodriguez (:crdlc) Assignee
	Comment 18 • 10 years ago

I am going to keep the UIs and messages like they are currently in master because this bug is "Hide regular Offline Error Screen When Connectivity Resumes - App Foreground and Background". The UIs and messages will be addressed in bugs related to "Offline error handling"

http://i.imgur.com/LNGb6eu.png

	Cristian Rodriguez (:crdlc) Assignee
	Comment 19 • 10 years ago

Messages will be modified in bug 969267 as they appear in the wireframe, thanks a lot

	Marcelino Veiga Tuimil [:sonmarce]
	Comment 20 • 10 years ago

As Cristian said, in this bug there is no change in the UI (also keeping retry button), because all of them will be applied in bug 969267, so after geting r+ we can land this bug as is

	Cristian Rodriguez (:crdlc) Assignee
	Comment 21 • 10 years ago

You are right, no changes on UI anymore here

	Cristian Rodriguez (:crdlc) Assignee
	Comment 22 • 10 years ago

Merged in master:

https://github.com/crdlc/gaia/commit/f1315076f1232ad9e00e548735022f81427550d4