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

Group by #317

Merged
merged 10 commits into from
Mar 5, 2018
Merged

Group by #317

merged 10 commits into from
Mar 5, 2018

Conversation

anjmao
Copy link
Member

@anjmao anjmao commented Mar 2, 2018

No description provided.

@anjmao anjmao requested a review from varnastadeus March 2, 2018 20:25
@anjmao
Copy link
Member Author

anjmao commented Mar 2, 2018

closes #263

@coveralls
Copy link

coveralls commented Mar 2, 2018

Coverage Status

Coverage increased (+0.5%) to 92.049% when pulling c3922c6 on group-by into 3a996f4 on master.


.ng-dropdown-panel-items {
.ng-optgroup {
user-select: none;
Copy link
Member

Choose a reason for hiding this comment

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

keep same height and padding for group

line-height: 3em;
height: 3em;
padding: 0 16px;

Copy link
Member Author

Choose a reason for hiding this comment

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

done

color: rgba(0, 0, 0, 0.38);
}
&.ng-option-child {
padding-left: 18px;
Copy link
Member

Choose a reason for hiding this comment

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

and set double the padding for child

padding-left: 32px;


private _groupBy(items: NgOption[], prop: string | Function): { [index: string]: NgOption[] } {
const groups = items.reduce((grouped, item) => {
const key = prop instanceof Function ? prop.apply(this, [item]) : item.value[prop];
Copy link
Member

Choose a reason for hiding this comment

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

can prop be a function in our case?

}

this._filteredItems = [];
term = searchHelper.stripSpecialChars(term).toUpperCase();
Copy link
Member

Choose a reason for hiding this comment

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

as I have looked, toLowerCase is much faster.
https://jsperf.com/javascript-touppercase-vs-tolowercase/2


this._filteredItems = [];
term = searchHelper.stripSpecialChars(term).toUpperCase();
for (const key of Object.keys(this._groups)) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe this can be simplified firstly filtering on flat structure items that match term:

this._filteredItems = this.items.filter(item => {
    const label = searchHelper.stripSpecialChars(item.label ? item.label.toString() : '').toLowerCase();
    return item.hasChildren || label.indexOf(term) > -1;
})

and then removing all groups that have no children?
Ideally it would be only two iterations

Copy link
Member Author

@anjmao anjmao Mar 3, 2018

Choose a reason for hiding this comment

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

I tried this version, but it is significant slower. In example with 5000 items there each item has one group(slowest possible scenario) I got result of 320ms for every filter call. This is because in second loop you will need check for each parent item if it has child items. Current implementation took only 13ms to complete. This is because you just need to search ones in each group and then only find head item and push it as first in group, also I'm not searching in group head items which is not needed.

@anjmao anjmao merged commit 53e4819 into master Mar 5, 2018
@anjmao anjmao deleted the group-by branch March 5, 2018 06:41
jakemdunn pushed a commit to jakemdunn/ng-select that referenced this pull request Oct 16, 2018
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.

None yet

3 participants