Closed
Bug 972567
Opened 10 years ago
Closed 9 years ago
Tooltip for Reconnect to Sync
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
VERIFIED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | verified |
People
(Reporter: rfeeley, Assigned: jessieh)
Details
(Whiteboard: [good first bug])
Attachments
(2 files, 2 obsolete files)
10.81 KB,
image/png
|
Details | |
2.41 KB,
patch
|
markh
:
review+
|
Details | Diff | Splinter Review |
The Reconnect to Sync Australis menu item could use a nice tooltip that says: Reconnect user@domain.com (substitute user@domain.com for the user’s email address)
Reporter | ||
Updated•9 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug]
Reporter | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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!
Flags: needinfo?(rfeeley)
Reporter | ||
Comment 3•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
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 :)
Flags: needinfo?(markh)
Assignee | ||
Comment 5•9 years ago
|
||
Alright, thanks for the help guys! I'll get to making the changes right away.
Assignee | ||
Comment 6•9 years ago
|
||
Alright, the changes have been made! The code builds and it works as intended; hopefully I've put this patch together properly.
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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!
Attachment #8613259 -
Attachment is obsolete: true
Attachment #8613348 -
Flags: review?(markh)
Comment 9•9 years ago
|
||
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.
Attachment #8613348 -
Flags: review?(markh)
Assignee | ||
Comment 10•9 years ago
|
||
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. :)
Assignee | ||
Comment 11•9 years ago
|
||
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.
Attachment #8613348 -
Attachment is obsolete: true
Attachment #8615527 -
Flags: review?(markh)
Comment 12•9 years ago
|
||
Comment on attachment 8615527 [details] [diff] [review] syncTooltip.patch - Third revision Awesome, thank you!
Attachment #8615527 -
Flags: review?(markh) → review+
Updated•9 years ago
|
Assignee: nobody → jessieh
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/243d64120baa
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment 16•9 years ago
|
||
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]
Updated•9 years ago
|
QA Whiteboard: [bugday-20150729]
Comment 17•9 years ago
|
||
(In reply to Saheda Reza [:Antora] from comment #16) > It's oke Latest Nightly and Firefox Beta. Awesome, thank you!
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•