node-red icon indicating copy to clipboard operation
node-red copied to clipboard

Added [activate] onClick, [menu] width, [onselect] cb

Open lgrkvst opened this issue 3 years ago • 2 comments

The additions should be pretty straight forwards, HOWEVER here's a discussion point:

I did remove the following:

if (val.trim() === "") {
   if (this.completionMenuShown) {
      this.menu.hide();
   }
   return;
}

My reasoning is that retaining the menu on empty string is a logical way to show the user the full set of options (metaphore being "the filter is nothing"). There's a conflict here, as emptying the field is probably what some users might do to get rid of the menu...

  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)

Proposed changes

Checklist

  • [x] I have read the contribution guidelines
  • [x] For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • [x] I have run grunt to verify the unit tests pass
  • [ ] I have added suitable unit tests to cover the new/changed functionality

lgrkvst avatar Jan 20 '22 21:01 lgrkvst

Must admit I would find it fairly annoying if when i click on an input it kept popping up a list of suggestions without me even starting to type... the clue is in the name auto-complete .

(edit) link ref from slack (while it remains) - https://node-red.slack.com/archives/C9RSU6S56/p1642544465008100

dceejay avatar Jan 21 '22 09:01 dceejay

There's a conflict here, as emptying the field is probably what some users might do to get rid of the menu...

It is a deliberate choice to hide the menu if the input is empty - so we don't want to change that behaviour.

Re onClick - from an API style, for other components we have (such as the popover), we use a property called trigger that defines what should cause the component to be activated. It helps to avoid lots of boolean onXYZ properties that are meant to be mutually exclusive, but then have to be checked etc.

So for this case, I would say if trigger === "click" add the click handler. That gives us plenty of room to add other types of trigger if needed. You should also use that check to disable the 'hide on empty' functionality. That keeps the behaviour the same for all existing uses of the component and will only change it for instances that enable the click trigger.

Re cb - property names should be descriptive (or at least, more so than cb). We already have onselect on the menu component we delegate to. I see no reason not to use onselect as the property name here for consistency.

knolleary avatar Jan 24 '22 21:01 knolleary