feat(ui5-multi-input): initial implementation #1942
Conversation
MapTo0
requested review from
d3xter666,
ilhan007,
vladitasev,
fifoosid and
hristop
- add test for multi input - correct input tests
There was a problem hiding this comment.
- I think it should use its own item, not directly tokens. Now it differs from select, combobox, multicombobox, input with suggestions in that regard (they all have their own items, and multiinput uses a generic token item)
- Looks like a low-level component that is intended for component authors, rather than for applications. If we ship it like this, apps would have to create a popover or other interface with the options when they click the button, they would have to manage deleting tokens, everything... I'd suggest we rename this to multiinputbase for example for users that want flexibility (f..e to create their own logic of deleting tokens, custom popover) and have a default implementation called multiinput that is fully functional. Or is it too similar to multicombobox if we did this?
In any case, even if we leave it at that, there should be at least one fully working example how people can take advantage of this component. I'm afraid there might be issues if they try implementing KBH and focus if we leave the details to them and only provide them the barebone tokenizer wrapper.
packages/main/src/Input.js
Outdated
| const popover = await this._getPopover(); | ||
|
|
||
| if (popover) { | ||
| popover.close(false, false, true); |
There was a problem hiding this comment.
We have almost the same function on the progotype, only the .close() parameters are different.
Please use that other function and modify it to pass all arguments to the .close() method
There was a problem hiding this comment.
focusout should be sync to get a proper handling of it. I did this method to prevent making it async
| * Indicates whether the tokenizer is expanded or collapsed(shows the n more label) | ||
| * @private | ||
| */ | ||
| expandedTokenizer: { |
There was a problem hiding this comment.
should not use the word "tokenizer" at all, this is a technical implementation, the users don't know what this means.
There was a problem hiding this comment.
This is a private property, any proposal for another name?
There was a problem hiding this comment.
In UI5 we have introduced the enumeration renderMode with values "Narrow" and "Loose", which we believe describe the "rendering behaviour" well. Also it allows for expansion in future with other modes.
packages/main/src/MultiInput.js
Outdated
| * @public | ||
| */ | ||
| tokens: { | ||
| type: Token, |
There was a problem hiding this comment.
we always put HTMLElement here and don't check for types any more in slots
| */ | ||
| tokens: { | ||
| type: Token, | ||
| multiple: true, |
There was a problem hiding this comment.
all slots are multiple, this is only for properties now, so I would suggest creating a default slot:
default: {
type: HTMLElement,
propertyName: "tokens"
}There was a problem hiding this comment.
Items is a default slot for input and its ancestors - MultiInput
There was a problem hiding this comment.
Find an Input with suggestions ("Input in Cozy" example within the Input test page):
- click in the input field
- type a letter to open the suggestions
- press Arrow Down
Instead of navigating within the list, the popover closes.
Use this example "Input suggestions with ui5-li"
- click in the input field
- type a letter to open the suggestions
- press Arrow Up
Instead of navigating within the list, the popover closes.
Use this example "Input suggestions with ui5-li", the same as above
- click in the input field
- type a letter to open the suggestions
- start pressing Arrow Down
After few successful navigations, the popover closes.
There was a problem hiding this comment.
(1) Is there a public event the users can listen to in order to react on those close buttons in the popover. I could see "value-help-icon-press" in the metadata only. in the code there is "token-delete" being fired and should be documented, because it is not inherited from the Input class.
(2) When you press a close button, as it takes the focus, you can see the "more" text is displayed, but this is not a huge issue
|
FIXES: #1852 |
No description provided.