Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMPASS-64: Provide Line Number Menus in CRUD for More Explicity Addition Actions #722

Merged
merged 10 commits into from Dec 29, 2016

Conversation

durran
Copy link
Member

@durran durran commented Dec 26, 2016

Note I've updated the description to show the latest screenshots in order to not have to scroll...
screen shot 2016-12-29 at 8 30 17 pm
screen shot 2016-12-29 at 8 30 29 pm
screen shot 2016-12-29 at 8 30 51 pm
screen shot 2016-12-29 at 8 31 01 pm
screen shot 2016-12-29 at 8 31 18 pm
screen shot 2016-12-29 at 8 31 42 pm
screen shot 2016-12-29 at 8 32 03 pm
screen shot 2016-12-29 at 8 32 12 pm
screen shot 2016-12-29 at 8 32 51 pm

@fredtruman
Copy link
Contributor

Because we're now using the button for adding a sibling field as well as an object/array child, I'm a little confused about what to expect. I also don't have the opportunity to add siblings to objects/arrays since this button is now used to add children.

It works out ok in the example gifs you have since the object/array come last in the list of fields and there's an ADD handy at the bottom of the list, but what if there were additional siblings to the object/array after and I wanted to add a new sibling between the object/array and those coming after?

I'm probably late to the game on this, but did we consider just adding an additional button for adding children to objects/arrays? This would preserve the existing functionality for adding siblings, and provide clues that these have the added functionality of adding children directly from the parent name, something like this maybe:

000
001

?

@durran
Copy link
Member Author

durran commented Dec 27, 2016

@fredtruman Actually adding a sibling element to an array or object is not currently possible in master, so adding this functionality is not removing any existing functionality. I like the idea of multiple buttons but I think having them both say "add" is even more confusing even with the icons proposed. I think "Add" makes sense for "Add [a child to this element]", and the other text should be something different, like "Insert [ an element after this element ]" but I don't like using the word "Insert" here as it could be cconfused with inserting the document. JS uses "insertAfter" and "appendChild" for DOM manipulation - maybe we could use these since the concepts are familiar?

@fredtruman
Copy link
Contributor

Yep - something like that makes sense for button text and I think we could tweak with some feedback to help differentiate them appropriately. I do like them as different buttons though.

Maybe since we don't have the add sibling after array/object we could just show but disable that button for now? Which is more confusing: showing and disabling "insert after" until we have it, or not showing it on objects/arrays at all?

@durran
Copy link
Member Author

durran commented Dec 27, 2016

I'll do an update here with all functionality and some button changes and ping you.

@durran
Copy link
Member Author

durran commented Dec 27, 2016

@fredtruman I updated the text and allow all functionality now:

screen shot 2016-12-27 at 8 43 02 pm

screen shot 2016-12-27 at 8 43 04 pm

screen shot 2016-12-27 at 8 43 08 pm

screen shot 2016-12-27 at 8 43 18 pm

@fredtruman
Copy link
Contributor

I like this. We can definitely test these buttons (from design side) to get the language and iconography right. 👍

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 27, 2016

I think it would be nice if we could keep the button for adding a child the same at both the parent and child level. To explain - to add a nested element right now, you can (1) click "Add Child" next to the Array or (2) click "Insert After" next to one of the children.

Maybe we can change the wording of the buttons to something along the lines of "Add Child" and "Add Field". That way, we can use the "Add Child" button in both cases, as seen below.

screen shot 2016-12-27 at 3 45 55 pm

screen shot 2016-12-27 at 3 46 09 pm

Also, "Insert After" doesn't necessarily make sense for the button that hangs at the end. Something more generic like "Add Field" would avoid this problem.
screen shot 2016-12-27 at 4 24 32 pm

Potential Bug

I noticed that pressing "Insert After" next to an Array will add a new string that appears nested when I think it shouldn't be. For example, I think lines 26 and 27 below should be at the same indentation level.

screen shot 2016-12-27 at 3 18 48 pm

@durran
Copy link
Member Author

durran commented Dec 27, 2016

@Sean-Oh Line 27 in your potential bug section looks like a style problem... That element is too far to the left - no field should be in the same gutter as the carats... My styling with your suggestions looks ok and the button clicking in all situations is where I expect:

screen shot 2016-12-27 at 10 46 13 pm

@durran
Copy link
Member Author

durran commented Dec 27, 2016

I've updated the button text as per @Sean-Oh 's suggestions and clicked like mad - all looks correct on my side:

screen shot 2016-12-27 at 10 46 13 pm

@rueckstiess
Copy link
Member

rueckstiess commented Dec 28, 2016

I've mapped out graphically what the current behavior now is. The use case here is nested objects inside an array. We should probably look at other cases as well (arrays in arrays, arrays in objects, objects in objects).

crud_hover_buttons

I find this quite complex, and a little confusing because buttons for the two cases "array" vs. "nested document" seem to have opposite meanings.

1. Array

This case is pretty clear to me.

  • add child adds another array element (we should probably talk about naming conventions, too. I find "element" clearer than "child" for arrays).
  • add field adds another field below this field

2. Object

This is more confusing to me.

  • add child adds a field inside the nested object. The only case where I think this button is useful is if the object is empty, otherwise I can just click on the add field button next to an existing field. Also, I don't associate the term "child" with fields in an object.

  • add field: this was totally unexpected to me. It adds another array element below the current array element. I would not call that a "field" under any circumstances.

So I think we are trying to find common button labels for two different use cases that require different button labels.

Trying to come up with clear rules when certain buttons need to appear:

  • Every (direct descendant) element in an array receives a add element below button. This will add another array element directly below the current one.
  • The array container itself has a add element button to insert an element at the first position (also fixes the empty array problem)
  • Every field in an object has a add field below button. This will add another sibling field directly below the current field.
  • The object container itself has a add field button to insert a field at the first position (also fixes the empty object problem).
  • buttons should be ordered by their scope: closer scope first. An object container button should come before the array element button.

@durran
Copy link
Member Author

durran commented Dec 28, 2016

Completely agree it's confusing. I think the main problems here are:

  1. CSS doesn't support nested elements, so it's not intuitive on how to handle any of this with the entire experience based on the dev tools from various browsers.

  2. In order to be explicit about the behaviour, we need to provide more verbose wording on what is actually going to happen when clicking. This creates a problem with the UI looking extremely cluttered with long text inside the buttons.

  3. Finding or creating icons to display visually what we want to convey in text is probably grounds for some sort of UX award.

I'm out of ideas at the moment. Will refresh and revisit.

@durran
Copy link
Member Author

durran commented Dec 28, 2016

And a quick thought before I go to bed... What if we scrap the buttons and stick the actions in the dropdowns on the right hand side? Then the dropdowns are more than a "type selection" list but general actions that can be taken on a single element in the document?

[ edit] And another thought... In spreadsheet applications users are used to clicking on the line numbers on the left side to insert/delete rows - maybe we could add a visual indicator on the line number boxes that when they click it brings up a contextual menu of options to handle this instead of the buttons...

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 28, 2016

Thank you for outlining some of the problems! I'll go back to the drawing board with this one and create some more options.

@durran We could also have a generic "Add" button only at the container level, but have it as a dropdown with all of the options. This would help avoid UI clutter.

@durran
Copy link
Member Author

durran commented Dec 28, 2016

@Sean-Oh @fredtruman @rueckstiess I just did a little prototype of what I was proposing:

screen shot 2016-12-28 at 4 00 15 pm

screen shot 2016-12-28 at 4 00 19 pm

screen shot 2016-12-28 at 4 00 23 pm

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 28, 2016

@durran The only potential problem I see with your prototype is discoverability. I don't think users will find it immediately intuitive that they can click on the line numbers. However, they'll probably be able to adapt very quickly after learning how to access the add options.

Another alternative:

We can have an Add button as a dropdown in a container element (array or object):

dropdown

And have a generic Add button next to any element within the container. This would create a sibling within the container:

screen shot 2016-12-28 at 11 01 01 am

@durran
Copy link
Member Author

durran commented Dec 28, 2016

I like that alternative, the only issue I see is when we have long strings that extend out to the types dropdown, we would need to allow enough space for the button and the menu. But I think that's solvable.

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 28, 2016

I agree. There's also a ticket to fix text overflow in editable documents (COMPASS-153) so maybe we can leave that problem out of this PR.

@rueckstiess
Copy link
Member

Another suggestion: actual right-click context menus. Electron has native support for this.

@fredtruman
Copy link
Contributor

So much behind what was once a single, seemingly innocent, button!

Just to +1 on what I think you were illustrating in your previous example @durran, GitHub has the single line comment function here:

screenshot 2016-12-28 12 34 10

That's probably a common enough pattern for us to take some inspiration from - at least some percentage of developers that use GitHub are able to discover that functionality.

There's only one function on this example - add a comment on a single line of code - so a plus is sufficient in their case. We could make it a text button, or some affordance for a dropdown menu.

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 28, 2016

@rueckstiess I think native right click would be the cleanest solution in terms of UI. However, I think it might be a bit strange to have native right click only work in the documents view during edit mode. If we roll it out, it should be an action that users can do universally throughout the app.

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 28, 2016

@durran
Also, regarding the bug about misaligned strings: you can reproduce by pressing "Add Field" next to an array container if the array is nested within an object.

bug

I've narrowed the problem to the renderChildren() function in editable-element.jsx:
I figure the element added below the nested array is treated as a child, and gets an indent={this.props.indent + 16}.

Elements added below the array get an additional padding-left of 16px, whereas elements added within the array get a padding-left of 32px. Really sorry, but I'm not sure how to fix this...

@durran
Copy link
Member Author

durran commented Dec 29, 2016

Ok an update with screenshots...

  1. Hover on a line changes the line number to a + with a dark grey box:

screen shot 2016-12-29 at 5 01 47 pm

  1. Clicking on the + for a normal element gives a single menu option with the field name:

screen shot 2016-12-29 at 5 02 03 pm

  1. Clicking on the + for an object gives 2 menu options with the field name for guidance:

screen shot 2016-12-29 at 5 02 23 pm

  1. Clicking on the + for an embedded object gives 2 menu options with the field name:

screen shot 2016-12-29 at 5 02 35 pm

  1. Clicking on the + for an array gives 2 menu options with the field name:

screen shot 2016-12-29 at 5 03 06 pm

  1. Clicking on the + for a array element gives 1 option with the element value:

screen shot 2016-12-29 at 5 07 03 pm

Still flushing out the styles but that's where I am heading now.

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 29, 2016

This looks awesome! Very clean and clear.

Two minor comments:
1.) Is it possible to have only one dropdown active at a time? Menus can get cluttered:
screen shot 2016-12-29 at 12 09 21 pm

2.) Users should be able click outside the dropdown menu to close it. Other dropdown menus in Compass allow this as well.

Thanks @durran!

@durran
Copy link
Member Author

durran commented Dec 29, 2016

@Sean-Oh Thanks for the comments - I will update with both suggestions.

@durran
Copy link
Member Author

durran commented Dec 29, 2016

Now only allows one menu open at a time, fixed styling of the line number box when element is edited/added/remove as well as the full row mouseover in these states.

screen shot 2016-12-29 at 7 38 50 pm

screen shot 2016-12-29 at 7 39 05 pm

screen shot 2016-12-29 at 7 39 10 pm

screen shot 2016-12-29 at 7 39 16 pm

screen shot 2016-12-29 at 7 39 22 pm

screen shot 2016-12-29 at 7 39 29 pm

screen shot 2016-12-29 at 7 42 37 pm

@durran durran changed the title COMPASS-64: Provide 'add' button for empty expanable addition COMPASS-64: Provide Line Number Menus in CRUD for More Explicity Addition Actions Dec 29, 2016
@durran durran merged commit e5b5501 into master Dec 29, 2016
@durran durran deleted the COMPASS-64 branch December 29, 2016 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants