Closed
Bug 1402929
Opened 7 years ago
Closed 7 years ago
Clearer labeling of overflow panel and its dropzone targets in customize mode
Categories
(Firefox :: Toolbars and Customization, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 58
People
(Reporter: abenson, Assigned: Gijs)
References
()
Details
(Whiteboard: [reserve-photon-structure])
Attachments
(4 files)
59 bytes,
text/x-review-board-request
|
mikedeboer
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mikedeboer
:
review+
|
Details |
55.46 KB,
image/jpeg
|
Details | |
15.32 KB,
patch
|
Gijs
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Have re-organized the design of the Overflow Panel and detailed some drop targets to help clarify how the panel can be used. https://mozilla.invisionapp.com/share/TJDNCM54F#/screens/255104764_Customize_-_Overflow_Panel_Updates
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon-structure] → [photon-structure][triage]
Updated•7 years ago
|
Flags: qe-verify+
Priority: -- → P4
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Assignee | ||
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
Priority: P4 → --
QA Contact: gwimberly
Whiteboard: [reserve-photon-structure] → [photon-structure][triage]
Updated•7 years ago
|
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Per discussion with Aaron, no grey bar thingy, but I made the other changes suggested. I put the header in its own commit because it's not upliftable. We can uplift the other change.
Assignee | ||
Comment 4•7 years ago
|
||
osx build for testing: https://queue.taskcluster.net/v1/task/Bpr2ujM0QL6OXXLAnMt8-A/runs/0/artifacts/public/build/target.dmg linux64: https://queue.taskcluster.net/v1/task/PF-BgF_ORSOe-AnqVgsaWw/runs/0/artifacts/public/build/target.tar.bz2 win64: https://queue.taskcluster.net/v1/task/U_gLxqQnSueD8mW9s4MpXQ/runs/0/artifacts/public/build/target.zip Aaron, can you check this matches your expectations? The items in the overflow panel currently have 12px padding everywhere, so I kept this for the header as well so they line up. I don't really want to risk changing everything to 10px now (for 57) given that the styling is shared with the hamburger panel etc. and it's been tricky to get items with & without icons to line up on all platforms in all cases. I also used relative font-sizing to get the header and description fonts. I think otherwise this matches the spec.
Flags: needinfo?(abenson)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8913879 [details] Bug 1402929 - try to make overflow panel in customize mode easier to understand, https://reviewboard.mozilla.org/r/185276/#review190516 This is awesome! Didn't expect this much of an improvement in usability... I have two non-blocking (no pun intended) comments. ::: commit-message-cd9c8:5 (Diff revision 1) > +Bug 1402929 - try to make overflow panel in customize mode easier to understand, r?mikedeboer > + > +This moves the explanatory text from a pseudoelement to a real element, > +and keeps only the image in a pseudoelement. It then uses a transitioned > +opacity to change to fade that image in and out. nit: to change or to fade? ::: browser/themes/shared/customizableui/customizeMode.inc.css:441 (Diff revision 1) > border-radius: var(--arrowpanel-border-radius); > %else > border: 1px solid var(--arrowpanel-border-color); > box-shadow: 0 0 4px hsla(0,0%,0%,.2); > %endif > + max-width: 29em; You don't have access to @wideMenuPanelWidth@ here?
Attachment #8913879 -
Flags: review?(mdeboer) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8913880 [details] Bug 1402929 - add header to overflow panel in customize mode, https://reviewboard.mozilla.org/r/185278/#review190520 Nice to have this separated out. r=me.
Attachment #8913880 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #5) > ::: browser/themes/shared/customizableui/customizeMode.inc.css:441 > (Diff revision 1) > > border-radius: var(--arrowpanel-border-radius); > > %else > > border: 1px solid var(--arrowpanel-border-color); > > box-shadow: 0 0 4px hsla(0,0%,0%,.2); > > %endif > > + max-width: 29em; > > You don't have access to @wideMenuPanelWidth@ here? Sadly, no, the define is in panelUI.inc.css (included in platform-specific panelUI.css), and this is in customizeMode.inc.css, which gets included in platform-specific browser.css . I'll file a followup to make this more sane. Could probably be a CSS variable.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
hg error in cmd: hg rebase -s 6fcf0a6fb13c -d 07e7579a07be: abort: can't rebase public changeset 6fcf0a6fb13c (see 'hg help phases' for details)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2c3125cbc8f5 try to make overflow panel in customize mode easier to understand, r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/023950f209ee add header to overflow panel in customize mode, r=mikedeboer
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8913879 [details] Bug 1402929 - try to make overflow panel in customize mode easier to understand, Approval Request Comment [Feature/Bug causing the regression]: Using overflow panel as the customizable panel in customize mode as part of photon. [User impact if declined]: confusing UX in customize mode when putting things in the panel. This used to be more obvious because the hamburger panel wasn't empty by default. [Is this code covered by automated tests?]: the UX isn't, the general functionality of dragging items to the panel is [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: verification the new UI/UX is in place would be good. No specific STR beyond opening customize mode and checking the experience of the drag/drop to/from the panel. Note that we explicitly decided not to implement the grey placement bar in the panel to indicate where an item would be dropped. [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: not too risky [Why is the change risky/not risky?]: it's a relatively small, frontend-only patch that changes the image positioning etc. in the overflow panel, as well as making it fade in/out when dragging items. There's pretty good automated test coverage of the general drag/drop interaction in customize mode, and not that many ways this code could interfere with the functionality - plus we still have quite a few weeks of beta left. [String changes made/needed]: not in this patch Note: APPROVAL REQUEST ONLY FOR THIS CSET, NOT THE SECOND ONE The second patch includes strings and won't make 57. In 57 we'll ship the new look without the "Overflow Panel" header, which will appear in 58. Still, the patch will be an improvement.
Attachment #8913879 -
Flags: approval-mozilla-beta?
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2c3125cbc8f5 https://hg.mozilla.org/mozilla-central/rev/023950f209ee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 17•7 years ago
|
||
Introducing customizeMode.overflowList.description wasn't really needed, since the content is the same as customizeMode.emptyOverflowList.description Having said that, is the name "Overflow Panel" now? Because it's been called "Overflow menu" so far in the UI https://transvision.mozfr.org/?recherche=Overflow+menu&repo=central&sourcelocale=en-US&locale=en-US&search_type=strings_entities
Flags: needinfo?(abenson)
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #17) > Introducing customizeMode.overflowList.description wasn't really needed, > since the content is the same as customizeMode.emptyOverflowList.description But we now use it when the list isn't empty, so the name was no longer correct, and on balance I decided that causing confusion for translators "forever" from now was worse than the 1-time pain of changing the label. Anyway, l20n is going to fix all of this pain, right...? > Having said that, is the name "Overflow Panel" now? Because it's been called > "Overflow menu" so far in the UI > https://transvision.mozfr.org/ > ?recherche=Overflow+menu&repo=central&sourcelocale=en-US&locale=en- > US&search_type=strings_entities --> Aaron?
Comment 19•7 years ago
|
||
(In reply to :Gijs from comment #18) > (In reply to Francesco Lodolo [:flod] from comment #17) > > Introducing customizeMode.overflowList.description wasn't really needed, > > since the content is the same as customizeMode.emptyOverflowList.description > > But we now use it when the list isn't empty, so the name was no longer > correct, and on balance I decided that causing confusion for translators > "forever" from now was worse than the 1-time pain of changing the label. > Anyway, l20n is going to fix all of this pain, right...? Ehm, not really ;-) A string ID is a string ID, so this case would remain the same. We're trying to find a way to solve "I want to change the string content, invalidating translations but without touching code and using a new ID", but that's hard and still on the planning table.
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8913879 [details] Bug 1402929 - try to make overflow panel in customize mode easier to understand, We should probably hold off uplifting this until bug 1405643 is fixed.
Attachment #8913879 -
Flags: approval-mozilla-beta?
Reporter | ||
Comment 21•7 years ago
|
||
Spoke to Gijs about the panel/menu issue. Menu is the more common term so we should change the title in the overflow "menu" to read Overflow Menu.
Flags: needinfo?(abenson)
Comment 22•7 years ago
|
||
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20171004100049 This issue has been verified on latest Firefox Nightly Build ID: 20171004100049 on Windows 8.1 x64, Mac OS 10.11 and Ubuntu 14.04. On Mac and Ubuntu all is ok. On Windows the "Overflow Panel" Headline is not bolded. This seems to be an issue compared with the rest of OSs where the Headline is bolded. There should be consistency between them. Is this expected?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 23•7 years ago
|
||
(In reply to Vlad Bacia-Mociran [:VladB] from comment #22) > On Windows the "Overflow Panel" Headline is not bolded. This seems to be an > issue compared with the rest of OSs where the Headline is bolded. There > should be consistency between them. > > Is this expected? I talked with Aaron and we think this is OK as-is. Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 24•7 years ago
|
||
Ni me to put a patch together that's upliftable with the relevant 2 csets (1 from here, 1 from bug 1405643, no strings).
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 25•7 years ago
|
||
Vlad, were there any other issues you found besides the bold state of the title, or is this verified on 58?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(vbacia.work)
Assignee | ||
Comment 26•7 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: Using overflow panel as the customizable panel in customize mode as part of photon. [User impact if declined]: confusing UX in customize mode when putting things in the panel. This used to be more obvious because the hamburger panel wasn't empty by default. [Is this code covered by automated tests?]: the UX isn't, the general functionality of dragging items to the panel is. [Has the fix been verified in Nightly?]: I believe so, see comment 25 (and any replies, maybe, by the time you read this). [Needs manual test from QE? If yes, steps to reproduce]: No specific STR beyond opening customize mode and checking the experience of the drag/drop to/from the panel, both when it's empty and when it's not empty. Note that we explicitly decided not to implement the grey placement bar in the panel to indicate where an item would be dropped. [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: not too risky [Why is the change risky/not risky?]: it's a relatively small, frontend-only patch that changes the image positioning etc. in the overflow panel, as well as making it fade in/out when dragging items. There's pretty good automated test coverage of the general drag/drop interaction in customize mode, and not that many ways this code could interfere with the functionality - plus we still have quite a few weeks of beta left. [String changes made/needed]: not in this patch Note: this combines the first cset from this bug and the first cset of bug 1405643. It doesn't have the second csets from those bugs because those contain strings.
Attachment #8915765 -
Flags: review+
Attachment #8915765 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 27•7 years ago
|
||
(In reply to :Gijs from comment #26) > [Has the fix been verified in Nightly?]: I believe so, see comment 25 (and > any replies, maybe, by the time you read this). Err, and comment 22. :-)
Comment 28•7 years ago
|
||
(In reply to :Gijs (not here until monday) from comment #25) > Vlad, were there any other issues you found besides the bold state of the > title, or is this verified on 58? I didn't encountered any other issues. The changes have been verified on on Nightly v58.0a1.
Flags: needinfo?(vbacia.work)
Assignee | ||
Updated•7 years ago
|
Comment on attachment 8915765 [details] [diff] [review] Patches for beta Photon related, Beta57+
Attachment #8915765 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 30•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/90069fb227e2
Comment 31•7 years ago
|
||
No issues were encountered while testing this (by following the guidelines provided in comment 26) on Firefox 57.0b9 (BuildId:20171016185129) using Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.11.6.
You need to log in
before you can comment on or make changes to this bug.
Description
•