Closed Bug 968828 Opened 11 years ago Closed 11 years ago

Sync Icon for the horizontal menu

Categories

(Firefox :: Sync, defect)

29 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: mmaslaney, Assigned: jaws)

References

Details

(Whiteboard: [Australis:P3][qa!])

Attachments

(5 files, 3 obsolete files)

Attached file Sync-horizontalbar.zip
Assets for the default and busy states
Blocks: 905997
Component: Toolbars and Customization → FxA
Whiteboard: [Australis] → [Australis] p=0
Whiteboard: [Australis] p=0 → [Australis] p=0, [qa+]
Attached patch Patch (obsolete) — Splinter Review
Thanks for the icons Michael. Tim, I've flagged you for review since this touches browser-syncui.js and I wanted to let you know that we'll now be setting the [status="active"] attribute on the #PanelUI-fxa-status button. Gijs, I followed the same naming scheme for the Windows icons as is being pursued in bug 960517.
Assignee: mmaslaney → jaws
Status: NEW → ASSIGNED
Attachment #8375318 - Flags: review?(ttaubert)
Attachment #8375318 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8375318 [details] [diff] [review] Patch Review of attachment 8375318 [details] [diff] [review]: ----------------------------------------------------------------- browser-syncui changes looks fine to me.
Attachment #8375318 - Flags: review?(ttaubert) → review+
Comment on attachment 8375318 [details] [diff] [review] Patch Review of attachment 8375318 [details] [diff] [review]: ----------------------------------------------------------------- r- because you're still referencing button even though it's no longer a local variable. ::: browser/base/content/browser-syncui.js @@ +108,5 @@ > + let button = document.getElementById(id); > + if (!button) > + return; > + button.removeAttribute("status"); > + }); Please file a followup bug about how this should interact with customization (if you customize the button away while syncing, it'll forever keep its [status] - but that was a bug before you fixed this). @@ +114,3 @@ > this._updateLastSyncTime(); > if (needsSetup) > button.removeAttribute("tooltiptext"); This will break now, please don't do that.
Attachment #8375318 - Flags: review?(gijskruitbosch+bugs) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Thanks for catching that extra `button` reference.
Attachment #8375318 - Attachment is obsolete: true
Attachment #8375598 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8375598 [details] [diff] [review] Patch v2 Review of attachment 8375598 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-syncui.js @@ +109,5 @@ > + if (!button) > + return; > + button.removeAttribute("status"); > + if (needsSetup) > + button.removeAttribute("tooltiptext"); But now we're removing the text from the Panel button as well... is that correct? :-\
Attachment #8375598 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v3Splinter Review
There is no tooltiptext applied to the panel button when it needs setup, so it was a no-op. But for defensive programming, if someone added a tooltip there in the future they may not expect it to get removed by this. Patch adjusted as we discussed on IRC.
Attachment #8375598 - Attachment is obsolete: true
Attachment #8375876 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8375876 [details] [diff] [review] Patch v3 Review of attachment 8375876 [details] [diff] [review]: ----------------------------------------------------------------- LGTM bar the nit below. ::: browser/base/content/browser-syncui.js @@ +114,2 @@ > > + if (needsSetup) Nit: if needsSetup && syncButton
Attachment #8375876 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch Patch for Aurora (obsolete) — Splinter Review
[Approval Request Comment] Bug caused by (feature/regressing bug #): new feature for Sync + Australis User impact if declined: wrong icon on the menu panel Testing completed (on m-c, etc.): locally, just pushed to fx-team Risk to taking this patch (and alternatives if risky): none expected String or IDL/UUID changes made by this patch: none
Attachment #8375892 - Flags: approval-mozilla-aurora?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
This doesn't look good on HiDPI OSX.
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8376295 - Flags: review?(mdeboer)
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis] p=0, [qa+] → [Australis:P3]
Comment on attachment 8375892 [details] [diff] [review] Patch for Aurora We should uplift the union of this and the patch I just added (assuming it gets review etc.)
Attachment #8375892 - Flags: approval-mozilla-aurora?
Comment on attachment 8376295 [details] [diff] [review] 968828-fix-sync-icon-osx-hidpi Review of attachment 8376295 [details] [diff] [review]: ----------------------------------------------------------------- That makes sense ;)
Attachment #8376295 - Flags: review?(mdeboer) → review+
Whiteboard: [Australis:P3] → [Australis:P3][qa+]
Attached patch Patch for AuroraSplinter Review
Attachment #8375876 - Attachment is obsolete: true
Attachment #8375892 - Attachment is obsolete: true
Comment on attachment 8376579 [details] [diff] [review] Patch for Aurora [Approval Request Comment] Bug caused by (feature/regressing bug #): Sync button/Australis User impact if declined: button doesn't have the correct icon Testing completed (on m-c, etc.): m-c, local Risk to taking this patch (and alternatives if risky): none String or IDL/UUID changes made by this patch: none
Attachment #8376579 - Flags: approval-mozilla-aurora?
Attachment #8375876 - Attachment is obsolete: false
Attachment #8376579 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [Australis:P3][qa+] → [Australis:P3][qa+] p=0
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis:P3][qa+] p=0 → [Australis:P3][qa+]
Keywords: verifyme
QA Contact: twalker
QA Contact: twalker
base state looks good with Nightly 20140218. It's a little trickier to catch a sync in progress.
Status: RESOLVED → VERIFIED
QA Contact: twalker
Keywords: verifyme
Whiteboard: [Australis:P3][qa+] → [Australis:P3][qa!]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: