Closed Bug 964724 Opened 7 years ago Closed 7 years ago

Hide regular Offline Error Screen When Connectivity Resumes - App Foreground and Background

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S1 (14feb)

People

(Reporter: crdlc, Assigned: crdlc)

References

Details

(Whiteboard: [systemsfe])

Attachments

(3 files)

No description provided.
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
Blocks: 915660, 915659
Whiteboard: [systemsfe]
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Status: tt is already implemented and right now I am working on unit tests
Attached file Patch v1
Hi Michael,

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

Thanks for your time mate
Attachment #8370083 - Flags: review?(mhenretty)
Target Milestone: 1.3 C3/1.4 S3(31jan) → 1.4 S1 (14feb)
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 :)
Thanks a lot for your help Michael
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?
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
Attached image framed offline.png
No retry button
Attachment #8371385 - Flags: ui-review?(fdjabri)
Attached image unframed offline.png
No retry button for apps
Attachment #8371386 - Flags: ui-review?(fdjabri)
Comments addressed! Thx
Flags: needinfo?(mhenretty)
Thanks again Cristian! I'll take a look at this next week.
Flags: needinfo?(mhenretty)
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
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.
Attachment #8370083 - Flags: review?(mhenretty)
Comment on attachment 8370083 [details]
Patch v1

Thanks, addressed comments.
Attachment #8370083 - Flags: review?(mhenretty)
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
Attachment #8370083 - Flags: review?(mhenretty) → review+
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.
Attachment #8371385 - Flags: ui-review?(fdjabri) → ui-review-
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.
Attachment #8371386 - Flags: ui-review?(fdjabri) → ui-review-
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.
Depends on: 911328, 911330, 915654
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
Messages will be modified in bug 969267 as they appear in the wireframe, thanks a lot
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
You are right, no changes on UI anymore here
Merged in master:

https://github.com/crdlc/gaia/commit/f1315076f1232ad9e00e548735022f81427550d4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.