Closed Bug 581623 Opened 12 years ago Closed 12 years ago

bring back some status UI

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: mconnor, Assigned: philikon)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
We've gone around and around on this, but I think this works until we have a better sense of Fx4 plans.

Key changes:

* 16x16 Sync icon in the status bar
** Has "Connect" label when not connected.  Clicking this will log you in. (one click, not two)
** No label when connected.  Clicking the icon will trigger a sync. (again, one click, not two)
** Throbber replaces icon when syncing/logging in (no text labels, but I think that's okay)
** If you want to disconnect, you can use the menu, but this is a fairly rare use-case.

* Notification titles are now displayed in the status bar next to the icon
** This makes the notification much more prominent/identified with Sync, for better or for worse.
Attachment #459986 - Flags: review?(philipp)
Comment on attachment 459986 [details] [diff] [review]
v1

I'm not a big fan of the implicit clickability of the "Connect" label. It's sort of a "Connect" button disguised as a label. But it's far better than having no indicator of connectedness at all.
Attachment #459986 - Flags: review?(philipp) → review+
Attached patch v2 wip (obsolete) — Splinter Review
* from feedback, adds (or tries to, not sure why it's not working) a tooltip using the lastSync label
* also hides Connect when setup is needed.  It should also probably call openPrefs in that instance.
Attachment #459986 - Attachment is obsolete: true
Assignee: mconnor → philipp
Attached patch v3 (obsolete) — Splinter Review
Improves on Mike's WIP as follows:

* When Sync isn't set up yet, clicking on the Sync icon will open the setup wizard.

* Display tooltip with last sync info as soon as it's connected. Don't show a tooltip when disconnected.
Attachment #460159 - Flags: review?(edilee)
Attachment #460041 - Attachment is obsolete: true
Duplicate of this bug: 578645
If it's going to act like a button, why not make it a toolbarbutton that people can customize? I don't think there's existing glyphs that change appearance (other than disabled), so it might be odd..

But potentially there could be a glyph for 1) not connected 2) have data to sync 3) in-sync (disabled?) where the behavior would be 1) connect then sync (or open account creation) 2) sync 3) nothing.

Re: sync throbber. Did Alex mention somewhere that the sync icon shouldn't be spinning anywhere?
I'm thinking along similar lines, Ed, but I think the focus of this bug is to fix the 1.4.x line. It all seems a bit elaborate for a stopgap, especially if we don't know what the Fx 4 plans are.
Attached patch fix OSX stylingSplinter Review
Fix padding issue on OSX in icon-only mode.
Attachment #460496 - Flags: review?(edilee)
Attached patch v4 (obsolete) — Splinter Review
Fix edge case: hide "Connect" label when user chooses "Use different account"
Attachment #460159 - Attachment is obsolete: true
Attachment #460844 - Flags: review?(edilee)
Attachment #460159 - Flags: review?(edilee)
Attachment #460496 - Flags: review?(edilee) → review+
Comment on attachment 460844 [details] [diff] [review]
v4

>+++ b/ui/firefox/content/sync.js
>+  updateUI: function updateUI() {
>+    if (window.location.href == getBrowserURL()) {
>+      let showLabel = !Weave.Service.isLoggedIn && !this._needsSetup();
>+      let button = document.getElementById("sync-status-button");
>+      button.setAttribute("class", showLabel ? "statusbarpanel-iconic-text" 
>+                                             : "statusbarpanel-iconic");
>+      button.image = "chrome://weave/skin/firefox/sync-16x16.png";
>+
>+      if (!Weave.Service.isLoggedIn) {
>+        // If we're not set up yet, the tooltip should read "Set up
>+        // sync" or similar, but we threw the string away in 1.4 :(
>+        button.removeAttribute("tooltiptext");
>+      }
Did we want to set the tooltiptext to Connect or Sync Now in various cases just like how you set the last sync time?

>+  handleSyncButton: function WeaveWin_handleSyncButton() {
>+    if (Weave.Service.isLoggedIn)
>+      this.doSync();
>+    else if (this._needsSetup())
>+      this.openSetup();
>+    else
>+      this.doLogin();
Do we want connect-n-sync? We don't have a string for that though. I suppose just a connect is good for now.. and we might trigger a sync anyway if the user hasn't synced for a while?

>   onNotificationAdded: function WeaveWin_onNotificationAdded() {
>+    for (let i = 0;i < notifications.length;i++) {
nit: fix up the spacing while you're modifying

>   onNotificationRemoved: function WeaveWin_onNotificationRemoved() {
>+    if (Weave.Notifications.notifications.length == 0) {
>+    else
>+      this.onNotificationAdded();
A note why you're doing this would be good. Making sure the right icon is shown etc. for the remaining notifications, yeah?
Attachment #460844 - Flags: review?(edilee) → review+
(In reply to comment #9)
> Did we want to set the tooltiptext to Connect or Sync Now in various cases just
> like how you set the last sync time?

Indeed.

> Do we want connect-n-sync?

Why?

> We don't have a string for that though. I suppose
> just a connect is good for now.. and we might trigger a sync anyway if the user
> hasn't synced for a while?

Correct.
(In reply to comment #10)
> > Do we want connect-n-sync?
> Why?
Feels like people will just as easily click the button multiple times until it starts spinning now. But I suppose it'll spin while connecting.
Attached patch v5Splinter Review
Addressed review comments.
Attachment #460844 - Attachment is obsolete: true
(In reply to comment #11)
> Feels like people will just as easily click the button multiple times until it
> starts spinning now. But I suppose it'll spin while connecting.

Yes. Not sure about that myself, but then again, the spinning icon is going away sooner or later anyway...
(In reply to comment #0)
> Key changes:
> 
> * 16x16 Sync icon in the status bar
Verified on 1.4.3b1
> ** Has "Connect" label when not connected.  Clicking this will log you in. (one
> click, not two)
Verified on 1.4.3b1
> ** No label when connected.  Clicking the icon will trigger a sync. (again, one
> click, not two)
Verified on 1.4.3b1
> ** Throbber replaces icon when syncing/logging in (no text labels, but I think
> that's okay)
Verified on 1.4.3b1
> ** If you want to disconnect, you can use the menu, but this is a fairly rare
> use-case.
Verified on 1.4.3b1
> 
> * Notification titles are now displayed in the status bar next to the icon
> ** This makes the notification much more prominent/identified with Sync, for
> better or for worse.
Verified on 1.4.3b1
Status: RESOLVED → VERIFIED
i'll also leave myself a comment to re-verify this after bug 571897 lands on trunk and see how what happens if anything crazy happens
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.