Closed
Bug 968828
Opened 11 years ago
Closed 11 years ago
Sync Icon for the horizontal menu
Categories
(Firefox :: Sync, defect)
Tracking
()
VERIFIED
FIXED
Firefox 30
People
(Reporter: mmaslaney, Assigned: jaws)
References
Details
(Whiteboard: [Australis:P3][qa!])
Attachments
(5 files, 3 obsolete files)
10.25 KB,
image/png
|
Details | |
9.60 KB,
application/zip
|
Details | |
23.71 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
1.09 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
26.74 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Will provide assets
Reporter | ||
Comment 1•11 years ago
|
||
Assets for the default and busy states
Updated•11 years ago
|
Blocks: fxdesktopbacklog
Updated•11 years ago
|
Whiteboard: [Australis] → [Australis] p=0
Updated•11 years ago
|
Whiteboard: [Australis] p=0 → [Australis] p=0, [qa+]
Assignee | ||
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
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 4•11 years ago
|
||
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-
Assignee | ||
Comment 5•11 years ago
|
||
Thanks for catching that extra `button` reference.
Attachment #8375318 -
Attachment is obsolete: true
Attachment #8375598 -
Flags: review?(gijskruitbosch+bugs)
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Comment 10•11 years ago
|
||
[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?
Comment 11•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment 12•11 years ago
|
||
This doesn't look good on HiDPI OSX.
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Updated•11 years ago
|
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Comment 13•11 years ago
|
||
Attachment #8376295 -
Flags: review?(mdeboer)
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis] p=0, [qa+] → [Australis:P3]
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
Comment on attachment 8376295 [details] [diff] [review]
968828-fix-sync-icon-osx-hidpi
Review of attachment 8376295 [details] [diff] [review]:
-----------------------------------------------------------------
That makes sense ;)
Updated•11 years ago
|
Attachment #8376295 -
Flags: review?(mdeboer) → review+
Updated•11 years ago
|
Whiteboard: [Australis:P3] → [Australis:P3][qa+]
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
Updated•11 years ago
|
Attachment #8375876 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8375892 -
Attachment is obsolete: true
Comment 18•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #8375876 -
Attachment is obsolete: false
Updated•11 years ago
|
Attachment #8376579 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Blocks: fxdesktopbacklog
Whiteboard: [Australis:P3][qa+] → [Australis:P3][qa+] p=0
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis:P3][qa+] p=0 → [Australis:P3][qa+]
Comment 20•11 years ago
|
||
Updated•11 years ago
|
Comment 21•11 years ago
|
||
Bustage fix on Aurora:
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/7c1316ed444b
Updated•11 years ago
|
QA Contact: twalker
Comment 22•11 years ago
|
||
base state looks good with Nightly 20140218. It's a little trickier to catch a sync in progress.
You need to log in
before you can comment on or make changes to this bug.
Description
•