"Set up Sync" link at bottom of home tab: mobile

VERIFIED FIXED in Firefox 10

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: rnewman, Assigned: ally)

Tracking

Trunk
Firefox 10
Dependency tree / graph

Details

(Whiteboard: [verfied in services])

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

7 years ago
Putting this in Sync UI for now.
(Reporter)

Updated

7 years ago
Blocks: 675826
Could be on our about:home, which is our start page (not new tab page) for now.
(Reporter)

Comment 2

7 years ago
Created attachment 555588 [details] [diff] [review]
WIP for Part 1.

This puts a couple of links, with localization entities, on the Fennec about:home page.
(Reporter)

Comment 3

7 years ago
Created attachment 555589 [details] [diff] [review]
WIP (untested) for importing WeaveGlue.

This hasn't even been run. Uploading for completeness.
Component: Firefox Sync: UI → General
Product: Mozilla Services → Fennec
QA Contact: sync-ui → general
Version: unspecified → Trunk
I think we want to say at least "Pair *this* Device" to incidate the difference between the "I want to connect this device to another device" and "There's this other device that I want to connect to this one".

Also, once the phone is paired, it can serve as a pairing partner, so we will also need "Pair a Device". At which point we should ask ourselves whether this isn't a bit confusing and we should get better strings for those two cases.
Summary: "Pair device" link at bottom of home tab: mobile → "Pair this Device" link at bottom of home tab: mobile
(Assignee)

Comment 5

7 years ago
Created attachment 562911 [details] [diff] [review]
WIP, largely saving for the formatting

We will eventually have a "Setup Sync on desktop". This patch is for potential future work when there will be two buttons. Both buttons currently do the same thing, prettily.
(Assignee)

Comment 6

7 years ago
Created attachment 562917 [details] [diff] [review]
Part 1 (v0) Pair Device on Mobile Home tab
Attachment #562917 - Flags: feedback?(philipp)
Attachment #555589 - Attachment is obsolete: true
Attachment #555588 - Attachment is obsolete: true
Comment on attachment 562917 [details] [diff] [review]
Part 1 (v0) Pair Device on Mobile Home tab

>       <div id="footer-wrapper">
>         <span id="feedback" style="width: &aboutHome.footerWidth;" class="section-row" pref="app.feedbackURL" onclick="openLink(this);" role="button">&aboutHome.giveFeedback;</span
>         ><span id="support" style="width: &aboutHome.footerWidth;" class="section-row" pref="app.support.baseURL" onclick="openLink(this);" role="button">&aboutHome.getHelp;</span>
>       </div>
>-
>+      <div id="sync-setup">
>+        <span id="syncAddDevice"
>+              class="section-row" style="width: &aboutHome.footerWidth;" 
>+              role="button"
>+              onclick="openPairADeviceWizard()"
>+              >
>+          &aboutHome.addDevice;
>+        </span>
>+      </div>

If the coding style were up to me, I'd prefer to put the 'style' attr on its own line and put the closing bracket (>) on the same line as the 'onclick' attr.

That said, the predominant style in this document is putting everything on the same line (see the <span>s in the footer-wrapper <div> above), and predominant style wins. So please adhere to that.

>@@ -483,11 +496,11 @@
>        }, false);
> 
>        // Giving a chance for any page transitions to happen, if any, before displaying the lightbox
>        setTimeout(function() {
>          document.getElementById("lightbox").style.display = "block";
>        }, 300);
>      }
>    }
>-  ]]></script>
>+   ]]></script>

Unrelated whitespace change. Please remove.

>+#sync-setup {
>+  font-size: 18px;
>+  margin-top: 24px;
>+  text-align: center;
>+}

This doesn't seem relevant to this patch. Left-over stuff?


Strings may be subject to change, of course. I'll talk to faaborg about them tomorrow. Looks good otherwise!
Attachment #562917 - Flags: feedback?(philipp) → feedback+
(Assignee)

Comment 8

7 years ago
> 
> >+#sync-setup {
> >+  font-size: 18px;
> >+  margin-top: 24px;
> >+  text-align: center;
> >+}
> 
> This doesn't seem relevant to this patch. Left-over stuff?
> 

'sync-setup' is the name of the div that contains the button, so it is still a relevant rule, as far as I understand. I know it doesn't look right if I remove it.
(In reply to :Ally Naaktgeboren from comment #8)
> 'sync-setup' is the name of the div that contains the button, so it is still
> a relevant rule, as far as I understand. I know it doesn't look right if I
> remove it.

Duh, you're right. Ignore me.
(Assignee)

Comment 10

7 years ago
Created attachment 562932 [details] [diff] [review]
Part 1 (v1) Pair Device on Mobile Home tab

Nits addressed. Philikon, should I just hold it until after the discussion tomorrow for the correct string?
Attachment #562917 - Attachment is obsolete: true
Spoke to Faaborg about the inconsistency of strings and the meaning of "Set up Sync" vs "Pair a Device":

* "Set up Sync" always means I'm configuring the local device for Sync.

* "Pair a Device" means I'm the master and I'm pairing with a second slave device that will receive my credentials. IN fact, we're going to call this "Pair a Device with an Activation Code" to make it crystal clear.

Eventually mobile will have both links, but for now only the first one is relevant (until we implement bug 624028).

So Ally, please change the string from "Pair a Device" to "Set up Sync". Also, can you provide a screenshot please? Thanks!
Summary: "Pair this Device" link at bottom of home tab: mobile → "Set up Sync" link at bottom of home tab: mobile
(Assignee)

Comment 12

7 years ago
philikon: Dumb question, but should the button disappear if sync is already set up on the device? Ultimately he pairing button will appear in its stead, but what about the intermediate state we are in now? 

The code as it exists now will not, but there is no feedback suggesting that I should change it. thoughts?
(Assignee)

Comment 13

7 years ago
Created attachment 563175 [details]
Screenshot of single button
(In reply to :Ally Naaktgeboren from comment #12)
> philikon: Dumb question, but should the button disappear if sync is already
> set up on the device? Ultimately he pairing button will appear in its stead,
> but what about the intermediate state we are in now? 
> 
> The code as it exists now will not, but there is no feedback suggesting that
> I should change it. thoughts?

Oh yeah, good point. Yes, the link should of course disappear when you already have Sync set up. The easiest way to test for that is to check whether the 'services.sync.username' pref is set and a non-empty string.
(Assignee)

Comment 15

7 years ago
Created attachment 563256 [details] [diff] [review]
Part 1 (v2) Pair Device on Mobile Home tab
Attachment #562911 - Attachment is obsolete: true
Attachment #562932 - Attachment is obsolete: true
Attachment #563256 - Flags: feedback?(philipp)
(Assignee)

Comment 16

7 years ago
Created attachment 563259 [details]
screenshot of issue after disconnect
(Assignee)

Comment 17

7 years ago
Comment on attachment 563256 [details] [diff] [review]
Part 1 (v2) Pair Device on Mobile Home tab

once the borked profile was removed, testing went fine. ready for review
Attachment #563256 - Flags: feedback?(philipp) → review?(philipp)
Comment on attachment 563256 [details] [diff] [review]
Part 1 (v2) Pair Device on Mobile Home tab

>+   function initSetupSync() {
>+    if (getChromeWin().Services.prefs.prefHasUserValue("services.sync.username")) {
>+      document.getElementById("syncSetupSync").style.display = "none";
>+    }
>+   }
>+
>+   function openSetupSyncWizard() {
>+     let chromeWin = getChromeWin();
>+     chromeWin.WeaveGlue.open();
>+   }

This is a good start, but not enough. If we get to the Sync wizard from the Home tab and then once it completes, we get back to it, it should no longer say "Set Up Sync". The link should disappear automatically, without the need for a manual reload. I think the best way is to observe the 'weave:service:setup-complete' notification, as well as 'weave:service:start-over'. The former will tell you that the device was just connected to Sync, the latter that the device was disconnected. In those observers you want to flip the visibility of the link accordingly. There's already examples for observers and how to clean them up properly in aboutHome.xhtml ({update|init|uninit}Addons).

> +<!ENTITY aboutHome.setupSync  "Setup Sync">

Our existing strings say: Set Up Sync (space between Set and Up.) See also: title of the bug :) (Also, I don't believe "setup" is actually a verb, it's a noun. But I may be wrong. Playing the ESL card here :))
Attachment #563256 - Flags: review?(philipp) → review-
(Assignee)

Comment 19

7 years ago
(In reply to Philipp von Weitershausen [:philikon] from comment #18)
> Comment on attachment 563256 [details] [diff] [review] [diff] [details] [review]
> Part 1 (v2) Pair Device on Mobile Home tab
> 
> >+   function initSetupSync() {
> >+    if (getChromeWin().Services.prefs.prefHasUserValue("services.sync.username")) {
> >+      document.getElementById("syncSetupSync").style.display = "none";
> >+    }
> >+   }
> >+
> >+   function openSetupSyncWizard() {
> >+     let chromeWin = getChromeWin();
> >+     chromeWin.WeaveGlue.open();
> >+   }
> 
> This is a good start, but not enough. If we get to the Sync wizard from the
> Home tab and then once it completes, we get back to it, it should no longer
> say "Set Up Sync". The link should disappear automatically, without the need
> for a manual reload. I think the best way is to observe the
> 'weave:service:setup-complete' notification, as well as
> 'weave:service:start-over'. The former will tell you that the device was
> just connected to Sync, the latter that the device was disconnected. In
> those observers you want to flip the visibility of the link accordingly.
> There's already examples for observers and how to clean them up properly in
> aboutHome.xhtml ({update|init|uninit}Addons).
> 
what about just reloading the page? it is probably simpler. would using observers buy us anything in terms of speed, performance, or memory usage?
(In reply to :Ally Naaktgeboren from comment #19)
> what about just reloading the page? it is probably simpler. would using
> observers buy us anything in terms of speed, performance, or memory usage?

For sure. Reloading the page creates flickering and lots of activity that we can easily avoid. Also, you would still have to know *when* to reload, right? So I'm not sure how you can get out of using observers in the first place.
(Assignee)

Comment 21

7 years ago
Created attachment 563638 [details] [diff] [review]
Part 1 (v3) Pair Device on Mobile Home tab
Attachment #563256 - Attachment is obsolete: true
Attachment #563259 - Attachment is obsolete: true
Attachment #563638 - Flags: feedback?(philipp)
Comment on attachment 563638 [details] [diff] [review]
Part 1 (v3) Pair Device on Mobile Home tab

>+      <div id="sync-setup">
>+        <a id="syncSetupSync" class="setup-link" href="#" onclick="openSetupSyncWizard();">&aboutHome.setupSync;</a>
>+      </div>

...

>+.setup-link { 
>+  text-decoration: underline; 
>+  color: blue; 
>+} 

Micro-optimization nit: a class rule will be slightly slower than an ID rule because it's an extra thing to check, so I suggest removing the "setup-link" class and making this a #syncSetupSync rule.

>+   function syncDisconnected() {
>+     /*todo double check that this is the correct way to turn the button back on*/
>+     document.getElementById("syncSetupSync").style.display = "inline";
>+   }

I don't see any other good way, but feel free to double check :)

>+#sync-setup {
>+  font-size: 18px;
>+  margin-top: 24px;
>+  text-align: center;
>+}
>+
>+.setup-link { 
>+  text-decoration: underline; 
>+  color: blue; 
>+} 

Can you upload a screenshot for faaborg to ui-r please?
Attachment #563638 - Flags: feedback?(philipp) → feedback+
(Assignee)

Comment 23

7 years ago
Created attachment 563654 [details]
Screenshot of single button
Attachment #563175 - Attachment is obsolete: true
Attachment #563654 - Flags: ui-review?(faaborg)
(Assignee)

Comment 24

7 years ago
Created attachment 563656 [details] [diff] [review]
Part 1 (v4) Pair Device on Mobile Home tab
Attachment #563638 - Attachment is obsolete: true
Attachment #563656 - Flags: review?(philipp)
Attachment #563656 - Flags: review?(philipp) → review+
Comment on attachment 563654 [details]
Screenshot of single button

works for me, consistent with the home tab on desktop.  perhaps not as high profile as we might like for conversion but we can worry about that later.
Attachment #563654 - Flags: ui-review?(faaborg) → ui-review+
Assignee: nobody → ally
(Reporter)

Comment 26

7 years ago
Verified and pushed to s-c on ally's behalf, because my machine finished an incremental build waaaaay faster than her MBA can do a clean build.

  https://hg.mozilla.org/services/services-central/rev/329e56f80534

I'll leave you guys to write up appropriate verification steps for QA to incorporate into their test suite.
Whiteboard: [fixed in services]
Attachment #563656 - Attachment is patch: true
on galaxy tab link correctly opens easy setup dialog.  The link is not present if Sync is setup already.
Whiteboard: [fixed in services] → [verfied in services]
https://hg.mozilla.org/mozilla-central/rev/329e56f80534
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
This bug is verified fixed on the latest Nightly build. However, the Set Up Sync and Tabs from your other computer buttons are doing the same thing. Should we duplicate this option on the about:home page?
IMO, I guess that it would be better that the name of "Tabs from your other computer" to be changed in "Set Up Sync" until the Sync is connected. After this step, the button name will be changed back to "Tabs from your other computer". 

--
Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111005
Firefox/10.0a1 Fennec/10.0a1
Device: Samsung Galaxy S
OS: Android 2.2
(In reply to Cristian Nicolae (:xti) from comment #29)
> IMO, I guess that it would be better that the name of "Tabs from your other
> computer" to be changed in "Set Up Sync" until the Sync is connected. After
> this step, the button name will be changed back to "Tabs from your other
> computer". 
> OS: Android 2.2

+1
(In reply to Cristian Nicolae (:xti) from comment #29)
> This bug is verified fixed on the latest Nightly build. However, the Set Up
> Sync and Tabs from your other computer buttons are doing the same thing.
> Should we duplicate this option on the about:home page?
> IMO, I guess that it would be better that the name of "Tabs from your other
> computer" to be changed in "Set Up Sync" until the Sync is connected. After
> this step, the button name will be changed back to "Tabs from your other
> computer". 
> 
> --
> Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111005
> Firefox/10.0a1 Fennec/10.0a1
> Device: Samsung Galaxy S
> OS: Android 2.2

This bug is fixed as filed. Please file a follow up bug and mark it blocking the set up tracking bug 675826. Thank you.
(In reply to Tracy Walker [:tracy] from comment #31)
> 
> This bug is fixed as filed. Please file a follow up bug and mark it blocking
> the set up tracking bug 675826. Thank you.

I've filled Bug 692104.
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111004
Firefox/10.0a1 Fennec/10.0a1
Device: Samsung Galaxy S
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.