Closed
Bug 1186521
Opened 9 years ago
Closed 9 years ago
Enforce new requirements for Hamburger menu badges
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: eoger, Assigned: eoger)
References
Details
(Whiteboard: [fxsync])
Attachments
(2 files, 2 obsolete files)
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
markh
:
review+
|
Details | Diff | Splinter Review |
Follow up on these comments : https://bugzilla.mozilla.org/show_bug.cgi?id=1183212#c1.
Concrete actions to be taken:
- Add update badge inside the Hamburger menu popup update button.
- Drop all badges on click on the Hamburger menu button.
Did I forget anything? CC :shorlander :markh
Assignee | ||
Comment 1•9 years ago
|
||
Comment 3•9 years ago
|
||
Comment on attachment 8638842 [details] [diff] [review]
bug-1186521.patch
Review of attachment 8638842 [details] [diff] [review]:
-----------------------------------------------------------------
Looking good, and the screen-shots are what I was expecting to see!
::: browser/base/content/browser.js
@@ +2593,5 @@
> PanelUI.menuButton.setAttribute("badge-status", badgeToShow);
> if (badgeToShow == "update-failed") {
> PanelUI.menuButton.setAttribute("badge", "!");
> }
> + PanelUI.panel.addEventListener("popupshowing", this, true);
I'm not sure the event listener state is being tracked correctly - eg, multiple addBadge calls, or many removeBadge calls with no corresponding addBadge calls seems likely to screw it up. I don't see a problem with always having the event listener exist (which might even mean you can just have the handler be inline and avoid the handleEvent function and the check it does for the correct event)
::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +542,5 @@
> position: relative;
> top: 25%;
> }
>
> +#PanelUI-update-status[update-status="succeeded"]::after {
it would be awesome if we could do something clever to avoid duplicating these rules, but I haven't thought too much about how we might go about that. Worth considering though.
Attachment #8638842 -
Flags: feedback?(markh) → feedback+
Assignee | ||
Comment 4•9 years ago
|
||
Comments addressed!
Attachment #8638842 -
Attachment is obsolete: true
Attachment #8639985 -
Flags: review?(markh)
Comment 5•9 years ago
|
||
Comment on attachment 8639985 [details] [diff] [review]
bug-1186521.patch
Review of attachment 8639985 [details] [diff] [review]:
-----------------------------------------------------------------
This looks fine to me pending shorlander's ui-review in bug 1180584.
Attachment #8639985 -
Flags: review?(markh) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Updated to use the new update-failed icon introduced in bug 1180584
Attachment #8639985 -
Attachment is obsolete: true
Attachment #8643194 -
Flags: review?(markh)
Updated•9 years ago
|
Attachment #8643194 -
Flags: review?(markh) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8643194 [details] [diff] [review]
bug-1186521.patch
>+#PanelUI-update-status[update-status]::after {
>+ content: "";
>+ width: 14px;
>+ height: 14px;
>+ margin-right: 16.5px;
>+ box-shadow: 0px 1px 0px rgba(255, 255, 255, 0.2) inset, 0px -1px 0px rgba(0, 0, 0, 0.1) inset, 0px 1px 0px rgba(12, 27, 38, 0.2);
>+ border-radius: 2px;
>+ background-size: contain;
>+ display: -moz-box;
>+}
>+
>+#PanelUI-update-status[update-status="succeeded"]::after {
>+ background-image: url(chrome://browser/skin/update-badge.svg);
>+ background-color: #74BF43;
>+}
>+
>+#PanelUI-update-status[update-status="failed"]::after {
>+ background-image: url(chrome://browser/skin/update-badge-failed.svg);
>+ background-color: #D90000;
>+}
Out of interest, what is this trying to achieve? I ask because I had to remove some uses of ::after in bug 1029937 and I wouldn't want you to trip over the same bug.
Comment 9•9 years ago
|
||
Edouard, can you please address comment 8 and see how applicable that bug is here?
Flags: needinfo?(edouard.oger)
Assignee | ||
Comment 10•9 years ago
|
||
I played with the window size and the inspector, wasn't able to reproduce this kind of bug.
Flags: needinfo?(edouard.oger)
Updated•9 years ago
|
Iteration: --- → 42.3 - Aug 10
Whiteboard: [fxsync]
Updated•9 years ago
|
Flags: firefox-backlog+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(rfeeley)
You need to log in
before you can comment on or make changes to this bug.
Description
•