Closed
Bug 868094
Opened 12 years ago
Closed 12 years ago
Confirm before destroying a sync connection
Categories
(Firefox for Metro Graveyard :: Sync, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: jimm, Assigned: jimm)
References
Details
Attachments
(5 files, 4 obsolete files)
186.02 KB,
image/png
|
Details | |
188.16 KB,
image/png
|
Details | |
702 bytes,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
700 bytes,
patch
|
Details | Diff | Splinter Review | |
11.96 KB,
patch
|
ally
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•12 years ago
|
||
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.
Comment 2•12 years ago
|
||
Pretty sure Rodrigo has posted a link to the CSS code for this progress indicator somewhere in a bug.
Comment 3•12 years ago
|
||
Yes, the details are on Bug 852622.
Assignee | ||
Comment 4•12 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 6•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 years ago
|
||
Comment 10•12 years ago
|
||
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•12 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 | ||
Comment 12•12 years ago
|
||
Attachment #745120 -
Attachment is obsolete: true
Attachment #745259 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #745655 -
Attachment description: part2 - disconnect warn and throbber → part1 - send obs event when startOver is complete
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #745657 -
Flags: review?(ally)
Comment 15•12 years ago
|
||
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 "Pair a device" 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•12 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.
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/14e581ecbf0c
https://hg.mozilla.org/mozilla-central/rev/616f2609c294
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Comment 18•12 years ago
|
||
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•12 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.
Comment 20•12 years ago
|
||
Thanks Jim, I read the discussion.
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•