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

Add resolveDelay Property to PeoplePicker #143

Closed
Arknev opened this Issue Sep 27, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@Arknev

Arknev commented Sep 27, 2018

Originally posted by @Arknev in #117 (comment)

The UI freezes when searching for users. In our tenant we have millions of users. It would be great if the "resolveDelay" property (IBasePickerProps) could be set when calling the PeoplePicker. I don't see a way to delay search until a minimum number of characters are entered, which would be best, but this time delay would likely meet our needs.

@tsekityam

This comment has been minimized.

Contributor

tsekityam commented Oct 4, 2018

Set resolveDelay to 1000 reduce the chance of no responding, but it doesn't resolve the issue.

The page may have no responding if I search for one character only. Looks like we need to find more way to improve the proformance

By the way, we can limit the length of text to be used in filtering by

  private _filterPersons(filterText: string): IPersonaProps[] {
    if (filterText.length < this.props.minFilterTextLength) {
        return [];
    }
    return this.state.peoplePersonaMenu.filter(item =>
    this._doesTextStartWith(item.text as string, filterText)
    || this._doesTextContains(item.text as string, filterText)
    || this._doesTextStartWith(item.secondaryText as string, filterText)
    || this._doesTextContains(item.secondaryText as string, filterText));
  }

But I think that it resolve the issue completely, because we still need to compare thousands of user profiles with the filter text.

I will try limiting the number of the result, see if it helps.

@tsekityam

This comment has been minimized.

Contributor

tsekityam commented Oct 4, 2018

Looks like limiting the number of suggestions helps.

Perhaps the people picker could not handle too many suggestions.

Here is the code I tried:

  /**
   * Filter persons based on Name and Email (starting with and contains)
   *
   * @param filterText
   */
  private _filterPersons(filterText: string): IPersonaProps[] {
    let result: IPersonaProps[] = [];
    let suggestionLimit = this.props.suggestionLimit ? this.props.suggestionLimit : 5;

    for (let i: number = 0; i < this.state.peoplePersonaMenu.length; i++) {
      let item = this.state.peoplePersonaMenu[i];

      if (this._doesTextStartWith(item.text as string, filterText)
      || this._doesTextContains(item.text as string, filterText)
      || this._doesTextStartWith(item.secondaryText as string, filterText)
      || this._doesTextContains(item.secondaryText as string, filterText))
      {
        result.push(item);
        if (result.length === suggestionLimit) {
          break;
        }
      }
    }

    return result;
  }
@AsishP

This comment has been minimized.

Contributor

AsishP commented Oct 5, 2018

@tsekityam , seems like above code is working because it is filtering/searching limiting to first 5 users. However this doesn't make the filter work properly as it should filter all users not 5. So, may be the above is not a right approach.

Looking at the technical resolution, I suppose there are two other approaches we could try and see if resolveDelay property is not a fix. @tsekityam , could you give these a try. I don't have an environment to test so cannot validate.

  1. Set limitResults property in _onPersonFilterChanged method to see if that fixes. I am thinking ResolveSuggestions method causing the bottleneck.
  2. Set resultsMaximumNumber property in BaseSuggestionprops to see if that limits the hang on the suggestion resolve.
@tsekityam

This comment has been minimized.

Contributor

tsekityam commented Oct 5, 2018

@tsekityam , seems like above code is working because it is filtering/searching limiting to first 5 users. However this doesn't make the filter work properly as it should filter all users not 5. So, may be the above is not a right approach.

The code in previous comment is for referencing only. I have created a pull request #145 for it.

Looking at the technical resolution, I suppose there are two other approaches we could try and see if resolveDelay property is not a fix. @tsekityam , could you give these a try. I don't have an environment to test so cannot validate.

1. Set `limitResults`property in `_onPersonFilterChanged`method to see if that fixes. I am thinking ResolveSuggestions method causing the bottleneck.

2. Set `resultsMaximumNumber` property in `BaseSuggestionprops` to see if that limits the hang on the suggestion resolve.

I will try them all later today.

@tsekityam

This comment has been minimized.

Contributor

tsekityam commented Oct 6, 2018

  1. Set limitResultsproperty in _onPersonFilterChangedmethod to see if that fixes. I am thinking ResolveSuggestions method causing the bottleneck.
  2. Set resultsMaximumNumber property in BaseSuggestionprops to see if that limits the hang on the suggestion resolve.

Both of them fix the freezing, seems that the limitation is on rending too many suggestions.

Anyway, we still need to add a new props resultsMaximumNumber to let user to set the number of results show.

By the way, limitResults is not a parameter of onResolveSuggestions callback, so it is always undefined. We have to change _onPersonFilterChanged to

  private _onPersonFilterChanged = (filterText: string, currentPersonas: IPersonaProps[]): IPersonaProps[] => {
    let limitResults = this.props.limitResults;
    if (filterText) {
      let filteredPersonas: IPersonaProps[] = this._filterPersons(filterText);
      filteredPersonas = this._removeDuplicates(filteredPersonas, currentPersonas);
      filteredPersonas = limitResults ? filteredPersonas.splice(0, limitResults) : filteredPersonas;
      return filteredPersonas;
    } else {
      return [];
    }
  }
@AsishP

This comment has been minimized.

Contributor

AsishP commented Oct 6, 2018

@tsekityam , great, thanks for checking it. Yes, adding the property resultsMaximumNumber should be fine but lets call the property 'maximumSuggestions' so it makes a closer sense to the implementation. Would you like to take a stab at this change and implement the changes, since you already have a pull request for it?

Yeah, _onPersonFilterChanged is a event method, so thought the value of limitResults should be picked automatically from suggestionProps but your implementation looks fine too. But lets' go with resultsMaximumNumber as we would't need to change any methods that way. Thoughts?

@tsekityam

This comment has been minimized.

Contributor

tsekityam commented Oct 6, 2018

@AsishP , agree. Let me revise my PR...

@estruyf

This comment has been minimized.

Collaborator

estruyf commented Oct 15, 2018

Thanks @tsekityam and @AsishP. I have merged the PR + updated the property to suggestionsLimit to make more sense of its intention.

A new beta has been released, if you want to test it out @Arknev to verify the fix. You can follow these steps in order to install a beta version: https://sharepoint.github.io/sp-dev-fx-controls-react/beta/

@estruyf estruyf added this to the 1.10.0 milestone Oct 15, 2018

@estruyf

This comment has been minimized.

Collaborator

estruyf commented Nov 6, 2018

This is now available in version 1.10.0.

@estruyf estruyf closed this Nov 6, 2018

@Arknev

This comment has been minimized.

Arknev commented Nov 15, 2018

Yes, this works great. One 'npm update', added new property (suggestionsLimit), bundled, and pushed to our site collection. Everything is working smoothly. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment