823450 - Data Connection warning dialog (Roaming Charge UI requirement for Shira)

Status: VERIFIED FIXED

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

Description

[skrishnan]

https://www.dropbox.com/sh/iij6lhwk1i06914/dO7TrfClsJ/Roaming%20UI_Dec%2013.pdf

GAIA changes for this feature

Comment 1

[skrishnan]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7106

Pointer to Github pull-request

Comment 2

[James Lal [:lightsofapollo]]

Comment on attachment 694315 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7106

Kaze can you review? It looks like :sri needs to clean up his commits first at minimum...

Comment 3

[Fabien Cazenave [:kaze]]

:sri, please ping me as soon as you’ve squashed / rebased your commits. It’s impossible to review as is.

Comment 4

[Fabien Cazenave [:kaze]]

:sri, note that when I have a trick git question, I usually ping :lightsofappollo. :-)

Comment 5

[skrishnan]

I rebased this , please let me know if that looks okay to you

Comment 6

[Fabien Cazenave [:kaze]]

This looks OK but I’m a bit worried by the fact that you’ve overwritten the common “dialogs” CSS rules in `ondataconnect.css': the point of those shared/style resources is to ensure a common look’n’feel on Gaia, I don’t think Casey meant to break it with these wireframes.

Comment 7

[Fabien Cazenave [:kaze]]

:sri, any reply to my last comment?

Comment 8

[Casey Yee]

Arun can you confirm that the intention here is to use the standard BB styling?

Comment 9

[Arun Balachandran Ganesan [:abc]]

Casey: 

Yes, the intention is to use the same dialog as in the existing building blocks so as to not require many changes.

:abc

Comment 10

[Fabien Cazenave [:kaze]]

Comment on attachment 694315 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7106

My remarks still haven’t been addressed (it’s been almost three weeks now)…
Please submit a new patch when you’re ready.

Comment 11

[Fabien Cazenave [:kaze]]

This has just been merged — despite my r- and the lack of any a+. Please backout ASAP.
https://github.com/mozilla-b2g/gaia/commit/a86ee423e1c7b0855551613fa801030ce1b94b5b

The really worrying part is this: 
> No reviews required for merges to shira (as per Kev Needham & Joe Cheng)
https://github.com/mozilla-b2g/gaia/pull/7106#issuecomment-12254508

Vivien, Kev, Joe: do you confirm this???

Comment 12

[Fabien Cazenave [:kaze]]

Looks like I overreacted as this patch is merged to the shira branch, not the default one.

I still find bizarre that patches can be merged with an r-.

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/7772

I've refined the original patch based on the comments in the other pull requests and request to merge it to the master branch.

Alive and Kaze, could you help review this patch to see if your comments addressed? Thanks a lot!

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 705794 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/7772

r+ for logic changed as our offline discusion:
* Stop polluting settingsDB
* Use dialog instead of attention screen

Comment 15

[Fabien Cazenave [:kaze]]

Comment on attachment 705794 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/7772

I month after the first patch, I think the emergency has gone and we should take another hour toseparate/factorize the code that is used to show the dialog box.

See the `openDialog' function in utils.js: we could either reuse it (that would require to add submit/reset attributes to the OK/Cancel buttons) or, if not possible, create a similar `openConfirm' function in utils.js that could be reused for other dialogs.

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 705794 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/7772

Kaze, I've changed it to use the "openDialog" function in util.js. Please help review it. As for the animation, I suggest we can merge this 1.0.1 feature at first and create another bug to re-design the transition related to "confirm" and "dialog". Thanks!

Comment 17

[Fabien Cazenave [:kaze]]

Arthur: we’re almost there, but I still have a few remarks that I’d like you to address please.

Comment 18

[Fabien Cazenave [:kaze]]

(see my comments on your pull requests)

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Kaze, please check the pull request. I've addressed your comments.

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Fixed. https://github.com/mozilla-b2g/gaia/commit/b65af11afd02e0cb7e536e97df8b422ed562f68b
Thank you kaze!

Comment 21

[Fabien Cazenave [:kaze]]

Looks good, thanks!

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

Reopen since this patch is not landed in v1-train.

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Axel, please be informed that this patch added the following IDs, thanks!

dataConnection-warning-head
dataConnection-warning-message
dataConnection-warning-notNow
dataConnection-warning-turnOn
stkAppsNotAvailable
simToolkit

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Landed on v1-train. https://github.com/mozilla-b2g/gaia/commit/c9682e988b37077635a7bf47268f1b99324c6501

Comment 25

[sarsenyev]

Build ID:20130215070202
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a9e4f8912607
Gaia   21ba59d933c66024cb351c2379315301d5352e0c
Kernel: Dec 5

Verified on "Unagi" Data Connection warning dialog shows up before switching to data roaming.