Improve working-set context menu UI/UX #6107
Conversation
|
@cocoa-coder, what do you think? I've used the cog, as suggested by @larz0 and a position absolute pushed to the right to have a consistent UI when the bar is resized and small...seems reasonable? |
|
A couple of screenshot to show the UI |
|
Wow nice. Could you try and make the cog smaller (try 13px) so that it's not larger than the "Working Files" text. Hope that makes sense. Thanks @artoale ~ |
|
Here we go :) |
| border: 1px solid transparent; | ||
| padding: 4px 6px; | ||
| .sprite-icon(0, 0, 13px, 13px, "images/topcoat-settings-13.svg"); | ||
| background-position:center; |
|
@cocoa-coder thanks for the comments! I've just pushed the updated code...since I'm not particularly good in UI/design/CSS any correction is much appreciated! |
|
Looking good !
I do the CSS corrections myself in a few hours (once I am free). Btw: Can you explain what you mean with: silent styling as the recent-project drop-down button ? |
|
"silent" was intended to mean "quiet" in topcoat terminology (there's no 2013/11/26 Cocoa-Coder notifications@github.com
Alessandro Artoni artoale.com |
| #working-set-option-btn { | ||
| position: absolute; | ||
| right: 4px; | ||
| top: 2px; |
There was a problem hiding this comment.
Let's change this to 7px so it lines up with the text on the left.
|
Thanks, I'll fix these as soon as I get home |
ghost
assigned 
|
@JeffryBooher to track |
|
Re #6012 disappearing: I have no idea what happened to it either, so fwiw I've pinged GitHub support. Once or twice in the past we've hit some sort of "stale cache" GH bug that lead to missing query results, but this seems different... |
| working_set_settings_cmenu.open({ | ||
| pageX: buttonOffset.left, | ||
| pageY: buttonOffset.top + buttonHeight | ||
| }); |
There was a problem hiding this comment.
This is an interesting way to do it -- totally different from the Recent Projects dropdown, but it actually seems cleaner. I think when @njx wrote the Recent Projects dropdown we didn't even have a context menu API yet, so that may be the only reason it doesn't also use this approach... But we should probably test a little to make sure the behavior is consistent since they are presented so similarly in the UI.
There was a problem hiding this comment.
This could easily become a context menu utility function where you just pass a menu and a dom query string and it can handle the rest for any menu that opens under a button/element. So the same implementation could be used for the Recent Projects menu.
There was a problem hiding this comment.
I'm not convinced: of course, having a shared implementation would be much better, but the change @TomMalbran is proposing may not be sufficient. The Recent Project plugin offer a "close" button that doesn't close the menu and, of course, remove the project. I don't think this kind of feature is general enough to be implemented directly in the ContextMenu, but we should come up with a redefined API (which would involve Menu too)
There was a problem hiding this comment.
That close button is to remove the projects from the menu, and not to close the menu, so it is a particular feature of the Recent projects menu, which doesn't even need to work with this code, and should stay where is implemented. This would be just to open and close both menus when clicking on the button/link.
There was a problem hiding this comment.
Yes, of course... What I'm saying is that the Recent Project menu cannot be
refactored to use Context Menu with this feature only
On Nov 28, 2013 8:17 PM, "Tomás Malbrán" notifications@github.com wrote:
In src/command/DefaultMenus.js:
*/$("#working-set-option-btn").on("click", function (e) {var buttonOffset,buttonHeight;
e.stopPropagation();
if (working_set_settings_cmenu.isOpen()) {working_set_settings_cmenu.close();} else {buttonOffset = $(this).offset();buttonHeight = $(this).outerHeight();working_set_settings_cmenu.open({pageX: buttonOffset.left,pageY: buttonOffset.top + buttonHeight});That close button is to remove the projects from the menu, and not to
close the menu, so it is a particular feature of the Recent projects menu,
which doesn't even need to work with this code, and should stay where is
implemented. This would be just to open and close both menus when clicking
on the button/link.—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6107/files#r7991026
.
There was a problem hiding this comment.
I am referring just to refactor, the open/close action when clicking on the project title.
There was a problem hiding this comment.
Sorry, but how can we refactor the Recent Project menu to use a shared
implementation if the implementation is inside Context Menu, which is not
used by the extension itself? Sorry, I'm probably missing something :-)
On Nov 28, 2013 8:27 PM, "Tomás Malbrán" notifications@github.com wrote:
In src/command/DefaultMenus.js:
*/$("#working-set-option-btn").on("click", function (e) {var buttonOffset,buttonHeight;
e.stopPropagation();
if (working_set_settings_cmenu.isOpen()) {working_set_settings_cmenu.close();} else {buttonOffset = $(this).offset();buttonHeight = $(this).outerHeight();working_set_settings_cmenu.open({pageX: buttonOffset.left,pageY: buttonOffset.top + buttonHeight});I am referring just to refactor, the open/close action when clicking on
the project title.—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6107/files#r7991133
.
There was a problem hiding this comment.
The utility function would be moved to the Menus file and made as a function that it exports. Then from the Default Menus and from the Recent Projects main file, that function can be called through the Menu module to create the required on click event.
There was a problem hiding this comment.
Mmm... Ok, I read "context menu" and got stuck with that idea :-) as soon
as I get back to my pc I'll try to refactor it out :-)
On Nov 28, 2013 8:44 PM, "Tomás Malbrán" notifications@github.com wrote:
In src/command/DefaultMenus.js:
*/$("#working-set-option-btn").on("click", function (e) {var buttonOffset,buttonHeight;
e.stopPropagation();
if (working_set_settings_cmenu.isOpen()) {working_set_settings_cmenu.close();} else {buttonOffset = $(this).offset();buttonHeight = $(this).outerHeight();working_set_settings_cmenu.open({pageX: buttonOffset.left,pageY: buttonOffset.top + buttonHeight});The utility function would be moved to the Menus file and made as a
function that it exports. Then from the Default Menus and from the Recent
Projects main file, that function can be called through the Menu module to
create the required on click event.—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6107/files#r7991328
.
|
@peterflynn, I've added tests for the new ContextMenu.prototype.isOpen method, but I'm quite sure you mean to test something else too...can you please specify what (and, if possible, in which spec file I should put them?) |
|
@artoale I meant more hand-testing the UI to make sure the behavior of Recent Projects vs. this new dropdown don't feel too inconsistent with each other. Given that the implementations are very different, there could easily be some subtle differences in behavior. As long as they're subtle that seems ok. I would look at things like the rollover state and popup styling, how they behave as the sidebar is resized smaller, which actions close vs. don't close the popup, how the menu items are styled and formatted (including on rollover), etc. One difference worth noting: I know a bunch of work went into making the Recent Projects dropdown keyboard-accessible. But most of our other popup menus are not keyboard-accessible (in fact they generally don't even take focus), so I wouldn't worry about replicating that behavior here... |
|
Definitely make sense... I'll try to test this as much as I can and
|
|
No need to go overboard with the fixes, it's just a good idea to test and make note of anything... if it's just little things they probably don't need to get addressed yet. |
|
Even better :-)
|
|
I've testing this feature for a while...and it seems to me that the only behavioral difference w.r.t the Recent Project extension is the key binding behavior. The context menu doesn't get focus therefore it's not keyboard-accessible. Apart from that, they both close much on every click (with a small exception being that the X for closing a recent project doesn't close the dropdown. Moreover, they both close when ESC is pressed. |
|
Some people expressed reservations about the new "Arrange Menu" because it would be a different style than the "Recent Projects" Menu. So for standardizing menus, here's a version of the recent files menu, with the new style applied. But I have to say, imo, no matter which one you favor, there should be a connection. ( or it may look like a context menu from windows 98 ) |
|
In my opinion the triangle is very oldish, while the plain dropdown is much
|
|
@cocoa-coder we don't use triangles for drop-down menus but we do use it for popovers and tooltips. I don't think we need to change how recent project drop-down works because it's more elegant right now. |
|
@TomMalbran, just to be sure I've got what you meant :) 8eb67f4 is a super-basic refactoring of the event handling registration. Does it look reasonable? |
|
@artoale Yes, something as simple as that is what I was thinking about. We could then see if that new function works for the Recent Projects menu. |
|
I was considering this more in details...well, it's not super straightforward: I think that, if we really want to refactor the recent project plugin to use this helper, we should consider a more in depth refactoring because:
long story short...it seems to me that refactoring Recent Project with this simple function creates more confusion and doesn't actually reduce code duplication: it requires in fact an adapter that provides a ContextMenu-compatible interface for RecentProject and we should still do a lot of manual event handling registration/registration. Moreover, since we don' t share neither the markup nor the hide/show logic, it will all results in a hackish solution rather than a more clean code. |
|
I am not talking about refactoring the entire code in the Recent Projects initialization, just use this function for the part that creates the menu and places it under the project name. |
|
Yes, I know...what I'm saying is that, by trying to do that, it looks more like a hack than a good reuse practice :) I' push soon a basic implementation and let you see what I mean :) |
|
ping :) |
|
Oh, sorry. We don't get mails when a new commit is done, so is better to add a reply after you add a comit and want feedback. That looks great, and was what I thought that we could refactor, just the opening and cloing using the mouse. If we find other stuff later that both codes share, we can refactor those later too. |
|
Here we go :) |
|
ping! |
|
@artoale can you merge master into this branch so I can look at this and merge it this week? Thanks! |
|
Yup, sure! I'll try to rebase it tomorrow night! @peterflynn, do you need it to be squashed too? |
|
@artoale you can squash if you want but we don't require external contributors to squash. |
This commit refers to issue adobe#6012 It rearrange items whitin the working sets and move all those related to "sorting" to a new drop down menu accessible through a new cog button in the working set header
|
@JeffryBooher , just completed the rebase...sry, but I didn't have time before now...cheers! |
|
Looks good...merging |
Improve working-set context menu UI/UX
|
🍻 Cheers! |
|
|
A couple of Fixes for PR #6107
| @@ -1036,6 +1054,25 @@ define(function (require, exports, module) { | |||
| $menus = testWindow.$(".dropdown.open"); | |||
| expect($menus.length).toBe(0); | |||
| }); | |||
|
|
|||
| it("check for context menu to have the right status", function () { | |||
This commit refers to issue #6012
It rearrange items whitin the working sets and move all those related
to "sorting" to a new drop down menu accessible through a new cog
button in the working set header