Confirm before destroying a sync connection

RESOLVED FIXED in Firefox 23

Status

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 23
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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?
Created attachment 744923 [details]
[Mockup] Disconnect sync prompt

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.
Created attachment 744927 [details]
[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.
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 745117 [details]
dot progress

cleaned up css.
Assignee: nobody → jmathies
(Assignee)

Comment 6

6 years ago
Created attachment 745120 [details]
dot progress

converted everything over to classes, and renamed the css class names so we can reuse this.
Attachment #745117 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
I wish we could get better aliasing on this. I wonder if there's some way to soften the outlines of the dots.
(Assignee)

Comment 8

6 years ago
Created attachment 745257 [details] [diff] [review]
part1 - send obs event when startOver is complete

I need to know when this is completed so I can display a throbber during the process.
Attachment #745257 - Flags: review?(rnewman)
(Assignee)

Comment 9

6 years ago
Created attachment 745259 [details] [diff] [review]
part2 - disconnect warn and throbber
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+
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Updated

6 years ago
Depends on: 852622
(Assignee)

Comment 12

6 years ago
Created attachment 745655 [details] [diff] [review]
part1 - send obs event when startOver is complete
Attachment #745120 - Attachment is obsolete: true
Attachment #745259 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #745655 - Attachment description: part2 - disconnect warn and throbber → part1 - send obs event when startOver is complete
(Assignee)

Comment 13

6 years ago
Created attachment 745656 [details] [diff] [review]
part2 - disconnect warn and throbber
(Assignee)

Comment 14

6 years ago
Created attachment 745657 [details] [diff] [review]
part2 - disconnect warn and throbber

cleaned up.
Attachment #745656 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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 16

6 years ago
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
Last Resolved: 6 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.
(Assignee)

Comment 19

6 years ago
(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.