Closed Bug 972567 Opened 6 years ago Closed 5 years ago
Tooltip for Reconnect to Sync
The Reconnect to Sync Australis menu item could use a nice tooltip that says: Reconnect email@example.com (substitute firstname.lastname@example.org for the user’s email address)
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug]
Hello! I'd like to tackle this as my first bug while I acquaint myself with the system here. Could you point me towards the appropriate files/directories that I should be looking at? I'm currently digging through the source tree, but even after reading the Mozilla Source Code Directory Structure guide I'm unsure where to start looking. I'm assuming it's somewhere in the browser folder, but I'm not 100% certain. Thanks!
Thanks Jessie! Let's get help from the Mark who worked on Sync originally and knows the code base more than I, a mere UX designer ;)
Flags: needinfo?(rfeeley) → needinfo?(markh)
In browser-fxaccounts, you will find a line: this.button.setAttribute("tooltiptext", userData.email); which is where the tooltip is set to just the user email address. You'll need to edit accounts.properties to add a new string - something like "reconnectDescription = Reconnect %S" (and with a "LOCALIZATION NOTE" comment documenting the %S, as you'll see already in the file for other strings. Then browser-fxaccounts.js will change to something like: let description = this.strings.formatStringFromName("reconnectDescription", [userData.email], 1); this.button.setAttribute("tooltiptext", description); ping me if you need more info - I'm "markh" on IRC, but note I'm in Australia, so timezones :)
Alright, thanks for the help guys! I'll get to making the changes right away.
Alright, the changes have been made! The code builds and it works as intended; hopefully I've put this patch together properly.
Comment on attachment 8613259 [details] [diff] [review] syncTooltip.patch Review of attachment 8613259 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks! Please fix the issue below and upload a new version and request review from me. ::: browser/base/content/browser-fxaccounts.js @@ +241,5 @@ > this.button.setAttribute("label", defaultLabel); > this.button.removeAttribute("tooltiptext"); > this.button.removeAttribute("fxastatus"); > > + let tooltipDescription = this.strings.formatStringFromName("reconnectDescription", [userData.email], 1); Please move this line to just above the setAttribute call that uses it - otherwise we are formatting the variable even when it's not used.
Attachment #8613259 - Flags: feedback+
Good catch! I didn't take notice of that when I was writing it out; I just thought it looked awfully nice nestled between those two blocks of code. ;) Here's a revised version of the patch with the variable definition in a more appropriate place, as suggested. Thanks again for all of the help!
Comment on attachment 8613348 [details] [diff] [review] syncTooltip.patch - Second revision Ack - I'm very sorry, but I misled you :( > this.button.setAttribute("tooltiptext", userData.email); is actually correct - that's the "successfully logged in" case - it's the block above - |if (this.loginFailed)| that needs changing - so you want your 2 new lines in the block above, and this block where the tooltip is set to the email address should be unchanged. Sorry about that. Note that to test the "login failed" case you probably want to create a new bool preference "services.sync.debug.ignoreCachedAuthCredentials" set to true, then change your FxA password, then force a sync - an info bar should appear at the bottom of the window asking you to reconnect, and the tooltip in the menu should adjust to this new "Reconnect you@somewhere" string.
Whoops! Sorry about that. I've swapped the code around and restored the previous tooltip for the "successfully logged in" case, and I'm now waiting for it to build. I'm going to be awfully busy tonight so I probably won't have the time to finish it up; I'll have the patch ready and submitted for review tomorrow afternoon. :)
Alright, I tested out the build I made last night and the tooltip changes properly when Sync is unable to login successfully. Here's the new revision of the patch with the proper changes.
Comment on attachment 8615527 [details] [diff] [review] syncTooltip.patch - Third revision Awesome, thank you!
Attachment #8615527 - Flags: review?(markh) → review+
I found same issue on nightly & also Latest Developer Edition.There didn't show tooltip on sync. Firefox nightly: Build ID 20140213030201 User Agent Mozilla/5.0 (Windows NT 6.3; rv:30.0) Gecko/20100101 Firefox/30.0 Firefox Developer Edition: Build ID 20150731004008 User Agent Mozilla/5.0 (Windows NT 6.3; rv:41.0) Gecko/20100101 Firefox/41.0 It's oke Latest Nightly and Firefox Beta. [buday-20150729]
(In reply to Saheda Reza [:Antora] from comment #16) > It's oke Latest Nightly and Firefox Beta. Awesome, thank you!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.