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.

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

feat(ui5-multi-combobox): introduce grouping functionality #5250

Merged
merged 3 commits into from
May 26, 2022
Merged

Conversation

niyap
Copy link
Contributor

@niyap niyap commented May 19, 2022

FIXES: #5060

@niyap niyap requested review from ilhan007, ndeshev and MapTo0 May 19, 2022 09:55
* @tagname ui5-mcb-group-item
* @public
* @implements sap.ui.webcomponents.main.IMultiComboBoxItem
* @since 1.0.0-rc.15
Copy link
Member

Choose a reason for hiding this comment

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

the since

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -1055,7 +1057,29 @@ class MultiComboBox extends UI5Element {
}

_filterItems(str) {
return (Filters[this.filter] || Filters.StartsWithPerTerm)(str, this.items);
const itemsToFilter = this.items.filter(item => !item.isGroupItem);
Copy link
Member

@ilhan007 ilhan007 May 19, 2022

Choose a reason for hiding this comment

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

At first glance it looks a little complicated these two methods.
Something that might help to look more straightforward is to build a map or other structure (probably onBeforeRendering) to store the connection between items and the groups they belong. And, when the user filters, it will be much less code, more readable, to get from that structure which group items should be displayed for which items that remained after filtering. But, this is more for the team review.

Copy link
Member

Choose a reason for hiding this comment

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

I think the same, Can we change the filters files to consider group items as that is implemented in both CB and MCB components?

Copy link
Member

Choose a reason for hiding this comment

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

as all combobox / mcb items and group items all implement the IComboBoxItem can we introduce a method e.g. #isGroupItem? or something like this?

@ilhan007
Copy link
Member

And there is a merge conflict packages/main/test/pages/MultiComboBox.html to be resolved

@ilhan007 ilhan007 self-requested a review May 19, 2022 14:30
ndeshev
ndeshev previously approved these changes May 23, 2022
*
* @private
*/
static _groupItemFilter(item, idx, allItems, filteredItems) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename the idx param to nextItemIdx for readibility

Copy link
Member

@MapTo0 MapTo0 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, the only thing that comes on my mind is that with huge collection of items grouping could have some performance impact as we are doing filtering twice now. If we optimise this we are good to go I believe.

@niyap niyap merged commit 597a6f2 into master May 26, 2022
@ilhan007 ilhan007 deleted the mcbgrouping branch June 2, 2022 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MultiComboBox: Grouping feature is missing
4 participants