Closed Bug 868094 Opened 11 years ago Closed 11 years ago

Confirm before destroying a sync connection

Categories

(Firefox for Metro Graveyard :: Sync, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(5 files, 4 obsolete files)

Spun off from bug 867201. The current disconnect button needs a user confirmation prompt of some sort.

Yuan, any suggestions on how to do this in the current (v1) UI?
Mockups for V1 and also V2(for comparison)
Tapping on the "Remove" button should perform the remove action. Tapping on cancel will dismiss the inline prompt, display "Remove this device" button.

We will use the indeterminate progress ring for syncing, adding, and removing device. In this case, the ring will appear to the left side of action buttons. 


The mockup is created based on the suggestion I made at Bug 868217. But I think this inline prompt can fit in with our current UI.
Attached image [Mockup] Disconnecting
Pretty sure Rodrigo has posted a link to the CSS code for this progress indicator somewhere in a bug.
Yes, the details are on Bug 852622.
(In reply to Rodrigo Silveira [:rsilveira] from comment #3)
> Yes, the details are on Bug 852622.

I think that code is going to need some tweaking. Smaller sizes seemed to make the dots too small.
Attached file dot progress (obsolete) —
cleaned up css.
Assignee: nobody → jmathies
Attached file dot progress (obsolete) —
converted everything over to classes, and renamed the css class names so we can reuse this.
Attachment #745117 - Attachment is obsolete: true
I wish we could get better aliasing on this. I wonder if there's some way to soften the outlines of the dots.
I need to know when this is completed so I can display a throbber during the process.
Attachment #745257 - Flags: review?(rnewman)
Comment on attachment 745257 [details] [diff] [review]
part1 - send obs event when startOver is complete

Review of attachment 745257 [details] [diff] [review]:
-----------------------------------------------------------------

We use :finish instead of :complete (there are utility functions that do the right thing, too, but no point here).
Attachment #745257 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 745257 [details] [diff] [review]
> part1 - send obs event when startOver is complete
> 
> Review of attachment 745257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We use :finish instead of :complete (there are utility functions that do the
> right thing, too, but no point here).

I'll update it to use 'finish' instead.
Depends on: 852622
Attachment #745120 - Attachment is obsolete: true
Attachment #745259 - Attachment is obsolete: true
Attachment #745655 - Attachment description: part2 - disconnect warn and throbber → part1 - send obs event when startOver is complete
cleaned up.
Attachment #745656 - Attachment is obsolete: true
Attachment #745657 - Flags: review?(ally)
Comment on attachment 745657 [details] [diff] [review]
part2 - disconnect warn and throbber

Review of attachment 745657 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits.

Go forth & ship!

::: browser/metro/base/content/browser.xul
@@ +418,4 @@
>        </setting>
> +      <vbox id="sync-disconnectwarnpanel" collapsed="true">
> +        <description id="sync-disconnectwarntitle">&sync.removewarn.title;</description> 
> +        <description id="sync-disconnectwarnmsg">&sync.removewarn.note;</description> 

nit: stray whitespace at the end of sync-disconnectwarnmsg & sync-disconnectwarntitle lines.

::: browser/metro/base/content/sync.js
@@ +454,5 @@
>      this._elements.errordescription.collapsed = true;
>  
>      let isConfigured = (!this._loginError && Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED);
>  
> +    // If we're in the process of disconnecting, we are not connected

I'd modify that comment that 'we are not connected' means we've called Weave.Service.logout()

::: browser/metro/locales/en-US/chrome/sync.dtd
@@ +13,5 @@
>  <!-- device name text edit -->
>  <!ENTITY sync.deviceName            "This device">
>  <!-- remove this device button -->
>  <!ENTITY sync.removebutton.label    "Remove this device">
> +<!ENTITY sync.removewarn.title      "Remove Metro Firefox from your Sync Account?">

Please use the &shortBrand names for metrofx & Sync here so that this localizes properly, updates with the correct branding for nightly/aurora/beta, and hurts less if we change the name of the product(s).

@@ +20,4 @@
>  
>  <!ENTITY sync.setup2.title          "Set up Sync">
>  <!ENTITY sync.setup.pair2           "To activate, select &#x0022;Pair a device&#x0022; on your other device.">
> +

nit: get rid of the space and move the sync.setup entities down with the others.

::: browser/metro/theme/browser.css
@@ +740,5 @@
> +  width: 22px;
> +  height: 22px;
> +}
> +
> +#syncdisconnectthrobber .progressBallInner {

thanks for writing css classes.

Descendant selectors are generally discouraged because they're slow. Well, these are certainly not the worst offenders in our code and this code won't run frequently, so it's not worth the time to make more performant(but fragile) ones.
Attachment #745657 - Flags: review?(ally) → review+
Comment on attachment 745657 [details] [diff] [review]
part2 - disconnect warn and throbber

Review of attachment 745657 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/browser.xul
@@ +427,5 @@
> +      </vbox>
> +      <vbox id="sync-disconnectthrobber" collapsed="false">
> +        <hbox>
> +          <spacer flex="1" />
> +          <cssthrobber id="syncdisconnectthrobber"></cssthrobber>

Nit: <cssthrobber/> works too.
https://hg.mozilla.org/mozilla-central/rev/14e581ecbf0c
https://hg.mozilla.org/mozilla-central/rev/616f2609c294
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
I was looking at sync.properties

     1.7  # LOCALIZATION NOTE: Used in the default os description when a new account
     1.8  # is being set up. Should be unique to Metro, so that it does not conflict
     1.9  # with Desktop. See /services/sync/modules/engines/clients.js locaName.
    1.10  sync.defaultAccountApplication=Metro %S
    1.11  
    1.12 +sync.disconnectPrompt=Remove Windows 8 style %S from your Sync Account?

As far as I understand both strings should be referring to the same object. Should sync.defaultAccountApplication be changed accordingly? Let me know if you think I should open a new bug for this.
(In reply to Francesco Lodolo [:flod] from comment #18)
> I was looking at sync.properties
> 
>      1.7  # LOCALIZATION NOTE: Used in the default os description when a new
> account
>      1.8  # is being set up. Should be unique to Metro, so that it does not
> conflict
>      1.9  # with Desktop. See /services/sync/modules/engines/clients.js
> locaName.
>     1.10  sync.defaultAccountApplication=Metro %S
>     1.11  
>     1.12 +sync.disconnectPrompt=Remove Windows 8 style %S from your Sync
> Account?
> 
> As far as I understand both strings should be referring to the same object.
> Should sync.defaultAccountApplication be changed accordingly? Let me know if
> you think I should open a new bug for this.

No need for a bug. There was a discussion about this on the metro mailing list. The text edit needed a short name that differentiated it from desktop, 'Metro' was chosen.
Thanks Jim, I read the discussion.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.