Closed
Bug 1354097
Opened 8 years ago
Closed 7 years ago
Add footer to overflow panel that opens customize mode
Categories
(Firefox :: Toolbars and Customization, enhancement, P1)
Tracking
()
People
(Reporter: Gijs, Assigned: bwinton)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-structure])
Attachments
(1 file)
This should be straightforward, but we should add a conventionally-styled footer to the overflow panel that opens customize mode.
Updated•8 years ago
|
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Updated•8 years ago
|
Whiteboard: [photon-structure] → [reserve-photon-structure]
Updated•7 years ago
|
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
Assignee | ||
Comment 2•7 years ago
|
||
I've got a patch in progress, so I'll grab this one so that no-one else starts working on it… :)
Assignee: nobody → bwinton
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8887061 -
Flags: review?(shorlander)
Reporter | ||
Comment 4•7 years ago
|
||
This duplicates a bunch of stuff from bug 1354086 and probably will run into the same test failures.
Assignee | ||
Comment 5•7 years ago
|
||
Oh, fantastic! I'll remove all of that stuff and let someone else worry about it. ;)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8887061 -
Flags: review?(shorlander)
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
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? :-)
Attachment #8887061 -
Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
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.
Attachment #8887061 -
Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
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!
Attachment #8887061 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 12•7 years ago
|
||
Re: the center-aligned text .. it should be left-aligned now. The spec is outdated and needs to be updated.
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Pushed by bwinton@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67be9d4a6c2c
Style and add a customize button to the overflow panel. r=Gijs, ui-r=shorlander.
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•7 years ago
|
Iteration: --- → 56.4 - Aug 1
Comment 16•7 years ago
|
||
Verified on Windows, Mac, and Ubuntu.
You need to log in
before you can comment on or make changes to this bug.
Description
•