:Gijs (he/him) Assignee
	Description • 11 years ago

The current icon isn't very big, because it was designed for use in toolbars. We should have a more high-resolution one, and/or decide if this icon fits with the other icons in the panel. Stephen, this probably needs your help (or that of someone else who can do the icon-wrangling for us).

(Filed based on bug 878544 comment 4, but I noticed this myself when looking at bookmark toolbar items + panel stuff)

	Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
	Comment 1 • 11 years ago

Looks hi-res to me. Is this asset still needed?

	:Gijs (he/him) Assignee
	Comment 2 • 11 years ago

(In reply to mmaslaney from comment #1)
> Created attachment 8345462 [details]
> Screen Shot 2013-12-10 at 12.15.21 PM.png
> 
> Looks hi-res to me. Is this asset still needed?

That's the bookmarks menu button, not the bookmarks toolbar item.

	u428464
	Comment 5 • 11 years ago

2 things to note :

This icon should also be updated in the "bookmarks sidebar" and the library.

The text is cropped a bit weirdly in customization mode. It should appear on two lines.

	:Gijs (he/him) Assignee
	Comment 6 • 11 years ago

(In reply to Guillaume C. [:ge3k0s] from comment #5)
> 2 things to note :
> 
> This icon should also be updated in the "bookmarks sidebar" and the library.

Why? The icon isn't changing outside the panel/palette. It'll be the same in toolbars as well. This bug is purely out of concern for the resolution of the icon when it's displayed at 36px^2. Certainly changing the icon everywhere doesn't need to happen as part of this patch. Furthermore, the flat icon will look out of place in the current bookmarks UI, so we'd have to refresh all of those icons, which is even more out of scope here.

	:Gijs (he/him) Assignee
	Comment 7 • 11 years ago

So this patch does fix the alignment of the icon by making palette wrappers remove and re-add flex for the things they wrap. I chose not to fix the text wrapping in here because we're still figuring out what the desired endstate is for cropped single-line vs. multiline labels in bug 944947.

	Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
	Comment 8 • 11 years ago

Updated with all platform themes.

	:Gijs (he/him) Assignee
	Comment 9 • 11 years ago

Comment on attachment 8360417 [details] [diff] [review]
Australis: Need a more high-res version of the bookmarks toolbar item placeholder icon for the palette/panel,

Cancelling review because I should integrate the other icons for Windows.

	:Gijs (he/him) Assignee
	Comment 10 • 11 years ago

So this adds the WinXP icon. I'm going to leave the win8 bit for Mike de Boer in bug 859751.

	Mike Conley (:mconley) (:⚙️)
	Comment 11 • 11 years ago

Comment on attachment 8360692 [details] [diff] [review]
Australis: Need a more high-res version of the bookmarks toolbar item placeholder icon for the palette/panel,

Review of attachment 8360692 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the little bug I found, I think this looks OK to me. Certainly better than what we had before.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +685,5 @@
>        toolbarItem.checked = true;
>      }
>  
> +    if (aWrapper.hasAttribute("flex") && !toolbarItem.hasAttribute("flex")) {
> +      toolbarItem.setAttribute(aWrapper.getAttribute("flex"));

This needs to be:

toolbarItem.setAttribute("flex", aWrapper.getAttribute("flex"))

	:Gijs (he/him) Assignee
	Comment 12 • 11 years ago

With my stupidity addressed (at least as far as this patch is concerned),

remote:   https://hg.mozilla.org/integration/fx-team/rev/4a5c84a4d166

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/4a5c84a4d166

	:Gijs (he/him) Assignee
	Comment 14 • 11 years ago

So this fixes the styling of the search bar and the bookmarks bar. There's still a separate issue that only occurs if moving the bookmarks toolbar items from the palette to the menu panel, but stops as soon as they've been in the toolbar. Don't think that needs to hold up this bug, though.

	:Gijs (he/him) Assignee
	Comment 15 • 11 years ago

Comment on attachment 8366666 [details] [diff] [review]
Backed out changeset 4a5c84a4d166 ()

Sigh. Wrong bug! :-)