Closed Bug 1354097 Opened 7 years ago Closed 7 years ago

Add footer to overflow panel that opens customize mode

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

53 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.4 - Aug 1
Tracking Status
firefox56 --- fixed
firefox57 --- verified

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.
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Moving to reserve backlog for now.
Priority: P2 → P3
Whiteboard: [photon-structure] → [reserve-photon-structure]
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
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
Status: NEW → ASSIGNED
Priority: P2 → P1
Attachment #8887061 - Flags: review?(shorlander)
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.  ;)
Attachment #8887061 - Flags: review?(shorlander)
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 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 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+
Re: the center-aligned text .. it should be left-aligned now. The spec is outdated and needs to be updated.
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.
https://hg.mozilla.org/mozilla-central/rev/67be9d4a6c2c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.4 - Aug 1
Blocks: 1387512
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: