-
Notifications
You must be signed in to change notification settings - Fork 486
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
Fixes #1285 - API proposal to ClayLabelsInputField component #1292
Conversation
96f556a
to
3ce8327
Compare
3ce8327
to
75c0f9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @matuzalemsteles, the component is amazing!! It's gorgeous and works perfectly!!
I've added some changes I'd like to see before we push this in. I'll summarize here:
- Create a
clay-dataprovider
package that can abstract configuring components that show data. Data can come locally or remotely. This could be attached to other components later such asTable
,List
,CardGrid
,Dropdown
... - Generalize the concept of what gets selected to
items
, rather than justlabels
. Maybe we should rename this component to something likemultiselect
oritemselector
... @victorvalle? - Keep hidden inputs that match the selected items values for standard form compatibility
- Investigate if we can reuse fuzzy search from some existing implementation
What do you think?
Nice job overall, let's keep this going! 💪
* @public | ||
* @return {Array} A list of items containing the corresponding characters | ||
*/ | ||
filterAutocomplete(data, query) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's extract this to something like clay-dataprovider
. See DataProvider for some reference or Kendo DataSource.
We should support both local and remote data. See also how we started doing it in clay-charts-shared and ClayBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really thinking about not dealing with the DataProvider here 😅, I'll separate a task to just deal with it. Thanks for the examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's get this component through and open an issue to abstract that!
|
||
<div {$attributes}> | ||
{if $label} | ||
<label>{$label}</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think <label>
here will be missing a11y attributes like for
<div class="input-group input-group-stacked-sm-down"> | ||
<div class="input-group-item"> | ||
<div data-keydown="{$_handleOnKeydown}" ref="formGroupInput" class="form-control dropdown-full form-control-tag-group"> | ||
{if $selectedLabels} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For easier integration, it might be good to have hidden input fields here with the label values. That way the component will be compatible with standard form submission.
We can't just select labels, they need to have values associated with them, so our input data should look more like:
data: [{
value: '123',
label: 'Element 1'
}, {
value: '456',
label: 'Element 2'
}]
On the UI, we should show the labels, but we would be updating and keeping a list of <input type="hidden">
with the values of the selected items. There should also be a way to configure the name of the input so the server will be able to retrieve the data as a native array as expected.
{@param? _removeFocusedLabel: any} | ||
{@param? contentRenderer: string} | ||
{@param? filteredItems: list<?>} | ||
{@param? selectedLabels: list<?>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this selectedItems
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had put them as items, but I had decided to change thinking it was not a good option 😅. Referring to items is better. Thanks Chema!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* @param {!String} string | ||
* @return {null|Object} | ||
*/ | ||
export const match = (query, string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @jbalsas, Yeah, I thought of using it here, but I had trouble getting to the Lexicon markup result, with the fuzzy it concatenates in the characters that are match the strong
tag, but we're having problems with the parameters that they accept HTML in Metal soy (metal/metal.js#381). And this utility is the same fuzzy implementation, but without the option to concatenate some markup, just list which items are active... so we can get to the desired https://github.com/liferay/clay/pull/1292/files#diff-087a55086d39969ce453305b7bca6fcfR160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good for now, then!
@jbalsas multiselect sounds good to me. It could be confused with other old pattern but we could even use this to replace that one. |
Doing some research I've found several instances of I'd say the SelectReact Select MultiSelectPicker |
I like the idea of going with
hey @jbalsas, thanks 💪. I'm going back to this in the next episode, I'll take care of adding some additional tasks with what we have here. |
I prefer mutli select when it can select multiple things. |
Let's go with |
I'm closing this to send the final changes in another PR. |
Disclaimer
I'm sending the API proposal to the new ClayLabelsInputField component. View the documents and Invision to analyze the use cases.
Proposal
The component does not manipulate the data or changes internal states, it only triggers events of what is happening and letting the developer decide what to do with the information or validate the information for example. but always respecting the principles that the Lexicon is predicting.
Events
The developer has the freedom to make decisions using the "life cycles" of the component, some of them are:
Helper methods
The component provides some auxiliary methods so that it can be used with events.
Array<string>
that corresponds toquery
.Extension points
Lexicon predicts that developers can customize dropdown items with a list of contacts, phones ... or customize the labels for example. You can start by creating a variation using soy exclusivity
deltemplate
to achieve the expected results.As the markup is still not 100% ready I added some css on the demo page to get closer to the Lexicon just for testing purposes, this will change when the markup is ready.