Fixes: Gracefully handle duplicate Node labels

benloh commented 7 months ago

This addresses #38 and #56

Disambiguate duplicate node labels

It was impossible to distinguish between nodes with identical labels while using autosuggest both using the Search field and while editing a node label.

We now show the node id along with the label to help differentiate between the nodes.

screenshot_1365

Improved autosuggest selection during Node label edit

During node label editing, the autosuggest behavior as confusing.

You couldn't easily trigger the autosuggest list if there was existing text -- you have to start typing to get the list to appear.
You couldn't easily dismiss the autosuggest list, which led to obscuring other node fields
Hovering over an item in the autosuggest list implied that you could click on the item to change the value, but during Node label edit, the autosuggest list intended to be purely informational, letting you know that there are matching node labels
The position of the autosuggest list was detached from the input field, making the relationship ambiguous
The autosuggest list had a flat white background which made it difficult to distinguish from the node background

The list is now improved:

Clicking in the label field will now immediately open the autosuggest list if there is matching text.
Clicking anywhere outside of the input field will now close the autosuggest list.
The autosuggest list no longer has a hover
The autosuggest list is now more obviously a pane attached to the input field.

screenshot_1371

Disambiguate source/target selection during Edge editing

When selecting a new source or target node while editing an edge, selecting a node that has identical labels would select the first matching label. If you try to link to the second one, the link will only be established to the first one.

We now link by using the node id field, so the correct node is always chosen.

Questions

Search autosuggest supports keyboard highlights, but not mouse Currently while using the Search field, if you use the keyboard arrow keys you can move the highlighted item around, the graph will display the currently highlighted item in light gray.

screenshot_1368

This does not work with a mouse. There is a secondary highlight via a mouse hover, but that does not highlight the node in the graph view. Should we add that? So we can support either mouse or keyboard?

Should the duplicate node warning ALWAYS be shown even while the input field does not have the focus?

Currently the duplicate node warning is only shown if the input field has the focus. So if for instance you open a duplicate node "abc", you will not see the duplicate node warning until you edit the node and click in the input field.

Should the warning ALWAYS be shown?
e.g. should you see "Duplicate node" when the node is open in VIEW mode? e.g. should you see "Duplicate node" when the node is open in EDIT mode and the label field does NOT have the focus?

Revise duplicate warning? Currently the default duplicate warning is:
```
You’re entering a duplicate node.  Do you want to View the Existing node, or Continue creating?
```
This is actually confusing and problematic from a UX perspective. You can't View the existing node currently. If we re-introduce the hover highlight via mouse we can potentially show the duplicate node in the graph view as you move your cursor around if that's helpful. But I think we don't want to introduce the ability to click on the highlight to change the name -- that'd only lead to inadvertently creating duplicate names. So what should the warning say?
```
"abc" node already exists
```

jdanish commented 7 months ago

Awesome, thanks!!

This does not work with a mouse. There is a secondary highlight via a mouse hover, but that does not highlight the node in the graph view. Should we add that? So we can support either mouse or keyboard?

Ideally, yes, it'd work with the mouse as well. If that will take more than an hour, though, let's make it an issue for later.
Also, the keyboard is a bit misleading in that I now expect enter to open the one I am moused over. If changing that would take an hour or more, let's make it an issue and leave it for later.

So what should the warning say?

What about this: instead of putting the warning in the drop-down, put it just below. Can we then use the sam property that determines if you have a cancel or delete button to have 1 of 2 warnings?

If this is a new node: "WARNING: at least one other node has the same name. Press cancel if you wish to look for the other(s) and edit them."
If this is an existing node: "NOTE: at least one other node has the same name. Use search or the node table to check the others."

And then if the warning is red, but the note is black it won't be as distracting on repeat visits.

Would that be doable without too much extra time? If it will take a while just use the second of the two.

benloh commented 7 months ago

This does not work with a mouse. There is a secondary highlight via a mouse hover, but that does not highlight the node in the graph view. Should we add that? So we can support either mouse or keyboard?

Ideally, yes, it'd work with the mouse as well. If that will take more than an hour, though, let's make it an issue for later.

Also, the keyboard is a bit misleading in that I now expect enter to open the one I am moused over. If changing that would take an hour or more, let's make it an issue and leave it for later.

It sounds like the ideal solution is to either use the keyboard OR the mouse but not both -- that way the behavior is consistent. I'm working on this.

So what should the warning say? What about this: instead of putting the warning in the drop-down, put it just below. Can we then use the sam property that determines if you have a cancel or delete button to have 1 of 2 warnings?

If this is a new node: "WARNING: at least one other node has the same name. Press cancel if you wish to look for the other(s) and edit them."

If this is an existing node: "NOTE: at least one other node has the same name. Use search or the node table to check the others." And then if the warning is red, but the note is black it won't be as distracting on repeat visits.

To keep things simple, I'm going to go with a single message. Especially because the Duplicate Warning message is defined in the template. (Note the screenshots are using the default template language, not the updated text you suggested. Just change the template to use whatever language you prefer. The new default template will use the language you suggested: "NOTE: At least one other node has the same name. Use search or the node table to check the others.").

In view mode, the duplicate message (gray text) is not visible.
In edit mode, the duplicate message (gray text) is shown.
When the label field has the focus, the duplicate warning (red, same text) is shown.

benloh commented 7 months ago

@jdanish The AutoSuggest list now supports either mouse or keyboard, and the functionality for both is now the same:

Highlighting an autosuggest line item either via mouse or keyboard will highlight the node in the graph
Clicking outside of the matchlist will close the list
Clicking on existing search text will immediately re-do the search
Clicking on the existing search text and editing the search text respects the current cursor position
Selecting a highlighted node from the matchlist will retain the search string state (not leave it blank, or replace the search string with the selected node label)

Getting all the nuances of the search for both the main search and the NCEdges source/target editing was a bit of a challenge because there were so many variations involved. But I think it's ready to test now.

jdanish commented 7 months ago

A few minor issues:

Carriage return works to create a new node even when not logged in and "new node" is grayed out (correctly)
If you use the mouse to select a suggested node, the id fills the search rather than the name. Does not appear to impact keyboard nav.

Step 1: Type name of existing node
Step 2: Click name from suggestion list
Note: the node ID is now in search field while correct node is displayed for view / editing

Also, if you shrink the window so that the "new node" button moves below the search field, the dropdown menu overlaps it (see attached). This one is minor and can be ignored if a big deal. If it's easy to move it to the right that'll work, though?

benloh commented 6 months ago

Carriage return works to create a new node even when not logged in and "new node" is grayed out (correctly)

Yikes. Sorry 'bout that. Fixed with aadb93d442c731eeb7a2b9320862c00a4ae0a83f

If you use the mouse to select a suggested node, the id fills the search rather than the name. Does not appear to impact keyboard nav.

Good catch. There were a ton of these. Should be fixed with 0843e1ee4f9d5c0d75f91bc295fd8fbb3222226f.

Also, if you shrink the window so that the "new node" button moves below the search field, the dropdown menu overlaps

Right justifying is a good idea. 49cadb9bbb0ea19a76fb3a095d8148bd8a3658c1

netcreateorg / netcreate-itest