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.

Sign up for GitHub

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 #1381 - Improvements clay multi select #1382

Merged
merged 10 commits into from
Dec 18, 2018
Merged

Fixes #1381 - Improvements clay multi select #1382

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c847008
Fixes #1381 - Adds the eventFromInput to emit with inputOnKeydown
matuzalemsteles Dec 10, 2018
60ff96c
Fixes #1381 - Add items and remove items by default and the new API f…
matuzalemsteles Dec 10, 2018
0149edb
Fixes #1381 - Update ClayAutocomplete tests
matuzalemsteles Dec 10, 2018
9915d67
Fixes #1381 - Update ClayMultiSelect tests
matuzalemsteles Dec 10, 2018
32eea13
Fixes #1381 - SF
matuzalemsteles Dec 11, 2018
0c21207
Fixes #1381 - Adds the new labelLocator and valueLocator APIs
matuzalemsteles Dec 13, 2018
f2d7334
Fixes #1381 - Replace the default value of filterField in favor of th…
matuzalemsteles Dec 13, 2018
6aca854
Fixes #1381 - Adds the data from the element filtered
matuzalemsteles Dec 13, 2018
2d56632
Fixes #1381 - Deprecates extractData and filterField and adds the lab…
matuzalemsteles Dec 13, 2018
1a60380
Fixes #1381 - Update tests
matuzalemsteles Dec 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/clay-autocomplete/src/ClayAutocomplete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ class ClayAutocomplete extends ClayComponent {
data: {
value: this.refs.input.value,
key: event.key,
eventFromInput: event.delegateTarget.tagName === 'INPUT',
},
name: 'inputOnKeydown',
originalEvent: event,
Expand Down
3 changes: 2 additions & 1 deletion packages/clay-autocomplete/src/__tests__/ClayAutocomplete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,9 @@ describe('ClayAutocomplete', function() {
expect(spy).toHaveBeenCalledWith(
expect.objectContaining({
data: {
value: 'foo',
eventFromInput: true,
key: 'o',
value: 'foo',
},
name: 'inputOnKeydown',
originalEvent: expect.any(Object),
Expand Down
61 changes: 6 additions & 55 deletions packages/clay-multi-select/demos/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,8 @@ <h2>Default State</h2>

<div class="row row-spacing">
<div class="col">
<h2>With autocomplete</h2>
<div id="autocomplete-block"></div>
</div>
</div>

<div class="row row-spacing">
<div class="col">
<h2>With autocomplete and interaction</h2>
<div id="interaction-block"></div>
<h2>With disabled autocomplete</h2>
<div id="disabled-autocomplete-block"></div>
</div>
</div>

Expand Down Expand Up @@ -96,66 +89,24 @@ <h2>With only data remote</h2>
spritemap,
}, '#default-block');

// With autocomplete
// With disabled autocomplete
new metal.ClayMultiSelect({
dataSource,
dataSource: [],
enableAutocomplete: false,
helpText: 'You can use a comma to enter tags',
label: 'Tags',
selectedItems,
spritemap,
}, '#autocomplete-block');

// With autocomplete and interaction
new metal.ClayMultiSelect({
dataSource,
label: 'Tags',
selectedItems,
spritemap,
events: {
itemAdded: (event) => {
event.target.selectedItems.push({label: event.data.label, value: event.data.label});
addToSelectedItems(event);
},
itemRemoved: removeItem,
itemSelected: (event) => {
const label = event.data.originalString.toLowerCase();
if (event.target.selectedItems.find(item => item.value === label)) return;
event.target.selectedItems.push({label, value: label});
addToSelectedItems(event);
},
},
helpText: 'You can use a comma to enter tags',
}, '#interaction-block');
}, '#disabled-autocomplete-block');

// With only data remote
new metal.ClayMultiSelect({
dataSource: 'https://api.pro.coinbase.com/currencies',
events: {
itemRemoved: removeItem,
itemSelected: (event) => {
const label = event.data.originalString.toLowerCase();
if (event.target.selectedItems.find(item => item.value === label)) return;
event.target.selectedItems.push({label, value: label});
addToSelectedItems(event);
},
},
extractData: (elem) => elem.name,
helpText: 'You can use a comma to enter tags',
label: 'Currencies',
spritemap,
}, '#selected-block');

function addToSelectedItems(event) {
event.target.selectedItems = event.target.selectedItems;
event.target.inputValue = '';
event.target.filteredItems = [];
}

function removeItem(event) {
event.target.selectedItems.splice(event.data.index, 1);
event.target.selectedItems = event.target.selectedItems;
}

</script>
</body>
</html>
53 changes: 37 additions & 16 deletions packages/clay-multi-select/src/ClayMultiSelect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ class ClayMultiSelect extends ClayComponent {
* Handle the click on the dropdown item and the propagation of the labelAdded event.
* @param {!Event} event
* @protected
* @return {Boolean} If the event has been prevented or not.
* @return {?Boolean} If the event has been prevented or not.
*/
_handleDropdownItemClick(event) {
return !this.emit({
data: event.data,
name: 'itemSelected',
originalEvent: event,
});
this.filteredItems = [];
this.inputValue = '';
this.refs.autocomplete.refs.input.focus();

return this._handleItemAdded(event.data[this.filterField], event, 'itemSelected');
}

/**
Expand All @@ -82,24 +82,30 @@ class ClayMultiSelect extends ClayComponent {

/**
* Continues the propagation of the itemAdded event.
* @param {!String} value
* @param {!Event} event
* @param {?String} eventName
* @protected
* @return {Boolean} If the event has been prevented or not.
* @return {?Boolean} If the event has been prevented or not.
*/
_handleItemAdded(event) {
const label = event.data.value.toLowerCase().replace(',', '');
_handleItemAdded(value, event, eventName = 'itemAdded') {
const label = value.toLowerCase().replace(',', '');

if (
label.trim() &&
!this.selectedItems.find(
itemSelected => itemSelected.label === label
)
) {
const index = this.selectedItems.push({label: label, value: label});

this.selectedItems = this.selectedItems;

return !this.emit({
data: {
label,
item: this.selectedItems[index - 1],
},
name: 'itemAdded',
name: eventName,
originalEvent: event,
});
} else {
Expand Down Expand Up @@ -160,12 +166,15 @@ class ClayMultiSelect extends ClayComponent {
*/
_handleItemRemoved(event) {
const index = event.getAttribute('data-tag');
const item = this.selectedItems[Number(index)];

this._removeFocusedItem();
this.selectedItems.splice(Number(index), 1);
this.selectedItems = this.selectedItems;

return !this.emit({
data: {
index: Number(index),
item,
},
name: 'itemRemoved',
originalEvent: event,
Expand All @@ -179,15 +188,17 @@ class ClayMultiSelect extends ClayComponent {
* @return {Boolean} If the event has been prevented or not.
*/
_handleOnInput(event) {
const value = event.data.value;

this._removeFocusedItem();

switch (event.data.char) {
case ',':
return this._handleItemAdded(event);
return this._handleItemAdded(value, event);
default:
return !this.emit({
data: {
value: event.data.value,
value,
},
name: 'queryChange',
originalEvent: event,
Expand All @@ -209,8 +220,8 @@ class ClayMultiSelect extends ClayComponent {
event.preventDefault();
if (this._itemFocused) {
return this._handleItemRemoved(this._itemFocused);
} else if (value) {
return this._handleItemAdded(event);
} else if (value && event.data.eventFromInput) {
return this._handleItemAdded(value, event);
}
break;
case 'Backspace':
Expand Down Expand Up @@ -332,6 +343,16 @@ ClayMultiSelect.STATE = {
*/
filteredItems: Config.array(Config.object()).value([]),

/**
* Set the field name that contains the string to be added when
* the item is selected.
* @default originalString
* @instance
* @memberof ClayMultiSelect
* @type {?string}
*/
filterField: Config.string().value('originalString'),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @jbalsas, I do not know if I understood your intention of the filterField, but I'm using it for the context of setting the field when the items are filtered.

https://github.com/liferay/clay/pull/1382/files#diff-21f70f10741963ea56df2ba593022af3R64

Copy link
Contributor

@jbalsas jbalsas Dec 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @matuzalemsteles, yeah, I think this does what's expected, but the initial value is odd. It should likely be value. I'd expect the itemSelected event from AutoComplete to emit the full item that was selected. This should be more than just a string, it should be the whole object. For example:

const items = [{
    foo: 1,
    bar: 2
}, {
    foo: 3,
    bar: 4
}];

You'd then set filterField = 'foo', and the value to add should then be 1 or 3 depending on which item got selected.

Additionally, we should have a counterpart for the label. We can't simply derive it from the value, as these things might not always have a clear mapping. Imagine the following:

const categories = [{
    categoryId: 10032,
    name: 'Food',
}, {
    categoryId: 203214,
    name: 'Animals',
}];

In this case, you'd likely want to set the field value to map to the categoryId field, and the label one to map the name field.

With this in mind, I'd propose something like:

const categories = [{
    categoryId: 10032,
    name: 'Food',
}, {
    categoryId: 203214,
    name: 'Animals',
}];

new MultiSelect({
    [...]
    // Both could be string|function
    labelLocator: 'name', // could be (item) => item.name
    valueLocator: 'categoryId', // could be (item) => item.categoryId
    [...]
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jbalsas for the clarifications, I'll make the modifications.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @jbalsas just clarifying something related to the filterField API, we are only using to get the field of the filtered item (See https://github.com/liferay/clay/blob/master/packages/clay-data-provider/src/ClayDataProvider.js#L152). I'm assuming a dev can make its own filter system and disable the default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the use of labelLocator and valueLocator of accepting functions is a bad thing because we need to pass to Soy and it does not deal well with this. I like the idea of ​​accepting only string, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, let's stick with strings for now, then 👌

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @jbalsas, yesterday I was looking at this and I did the following to be able to accept the string and function, I kept your idea and I'm just not taking into consideration to modify the selectedItems API schema, that way I just do the mapping of labelLocator and valueLocator... What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you do? 😂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha 😅, I'm going to send what I had done, it would have been clearer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😂 👍


/**
* Help text to guide the user in the interaction.
* @default undefined
Expand Down
30 changes: 24 additions & 6 deletions packages/clay-multi-select/src/__tests__/ClayMultiSelect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,10 @@ describe('ClayMultiSelect', function() {
expect(spy).toHaveBeenCalledWith(
expect.objectContaining({
data: {
label: 'foo',
item: {
label: 'foo',
value: 'foo',
},
},
name: 'itemAdded',
originalEvent: expect.any(Object),
Expand All @@ -297,7 +300,10 @@ describe('ClayMultiSelect', function() {
expect(spy).toHaveBeenCalledWith(
expect.objectContaining({
data: {
label: 'bar',
item: {
label: 'bar',
value: 'bar',
},
},
name: 'itemAdded',
originalEvent: expect.any(Object),
Expand Down Expand Up @@ -330,7 +336,10 @@ describe('ClayMultiSelect', function() {
expect(spy).not.toHaveBeenCalledWith(
expect.objectContaining({
data: {
label: 'foo',
item: {
label: 'foo',
value: 'foo',
},
},
name: 'itemAdded',
originalEvent: expect.any(Object),
Expand Down Expand Up @@ -367,7 +376,10 @@ describe('ClayMultiSelect', function() {
expect(spy).toHaveBeenCalledWith(
expect.objectContaining({
data: {
index: 1,
item: {
label: 'Bar',
value: 'bar',
},
},
name: 'itemRemoved',
originalEvent: expect.any(Object),
Expand Down Expand Up @@ -415,7 +427,10 @@ describe('ClayMultiSelect', function() {
expect(spy).toHaveBeenCalledWith(
expect.objectContaining({
data: {
index: 1,
item: {
label: 'Bar',
value: 'bar',
},
},
name: 'itemRemoved',
originalEvent: expect.any(Object),
Expand Down Expand Up @@ -461,7 +476,10 @@ describe('ClayMultiSelect', function() {
expect(spy).toHaveBeenCalledWith(
expect.objectContaining({
data: {
index: 1,
item: {
label: 'Bar',
value: 'bar',
},
},
name: 'itemRemoved',
originalEvent: expect.any(Object),
Expand Down