|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
This should be straightforward, but we should add a conventionally-styled footer to the overflow panel that opens customize mode.
Moving to reserve backlog for now.
I've got a patch in progress, so I'll grab this one so that no-one else starts working on it… :)
This duplicates a bunch of stuff from bug 1354086 and probably will run into the same test failures.
Oh, fantastic! I'll remove all of that stuff and let someone else worry about it. ;)
Comment on attachment 8887061 [details] Bug 1354097 - Style and add a customize button to the overflow panel. , ui-r=shorlander. https://reviewboard.mozilla.org/r/157804/#review163408 ::: browser/components/customizableui/content/panelUI.inc.xul:420 (Diff revision 2) > overflowfortoolbar="nav-bar"/> > <toolbarseparator id="widget-overflow-fixed-separator" hidden="true"/> > <vbox id="widget-overflow-fixed-list" class="widget-overflow-list" hidden="true" > emptylabel="&customizeMode.emptyOverflowList.description;"/> > </vbox> > + <menuitem command="cmd_CustomizeToolbars" This should be a toolbarbutton. Is there a styling reason to use a menuitem? :-)
Comment on attachment 8887061 [details] Bug 1354097 - Style and add a customize button to the overflow panel. , ui-r=shorlander. https://reviewboard.mozilla.org/r/157804/#review164696 This is very tidy - thanks! Still some comments, because what would life be without comments? ;-) ::: browser/components/customizableui/content/panelUI.inc.xul:426 (Diff revision 3) > #ifdef MOZ_PHOTON_THEME > + <toolbarbutton command="cmd_CustomizeToolbars" > + id="overflowMenu-customize-button" > + class="subviewbutton panel-subview-footer" > + accesskey="&viewCustomizeToolbar.accesskey;" > + label="&viewCustomizeToolbar.label;"/> Interestingly, the mocks say "Customize Toolbar..." . This says "Customize..." . Was that a deliberate change? More generally, we probably need a new string and access key anyway, because in the view menu context is different so different access keys may or may not be available. Finally, in the mocks ( https://mozilla.invisionapp.com/share/BXBFSCU6P#/screens/229252091_Customization ) the footer text is centered. Not sure how tricky that is, but I think it would make sense to do that here.
Comment on attachment 8887061 [details] Bug 1354097 - Style and add a customize button to the overflow panel. , ui-r=shorlander. https://reviewboard.mozilla.org/r/157804/#review165936 Huh, so I am a bit of an idiot, because I didn't realize literally every. other. subview. footer. doesn't center text. Which makes me wonder if the centering here is intentional or not... So, um, r=me for this version, but please check with Bryan / Aaron / Stephen if this is correct, or if the footer here should just be left-aligned like the other ones, and then you can remove the rule I just made you add. Sorry for the hassle!
Re: the center-aligned text .. it should be left-aligned now. The spec is outdated and needs to be updated.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/67be9d4a6c2c Style and add a customize button to the overflow panel. r=Gijs, ui-r=shorlander.
Verified on Windows, Mac, and Ubuntu.