sp-dev-fx-controls-react icon indicating copy to clipboard operation
sp-dev-fx-controls-react copied to clipboard

PeoplePicker Control WebPart Context

Open christopher-walker opened this issue 5 years ago • 9 comments

Category

[ ] Enhancement

[ ] Bug

[x] Question

Version

Please specify what version of the library you are using: [1.16.0]

Question

The people picker control requires the entire webpart context to be passed down into the form. I am trying to clean my code up and create tests to cover as much of the code as possible. Is there any particular reason for this? from what I can see the PeopleSearch service needs the absoluteUrl of the web and the HTTP Client. I was planning on taking a further look into the code and maybe chaning these? is there any reason why this should not be done?

Thanks!

Chris

christopher-walker avatar Jan 27 '20 21:01 christopher-walker

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

ghost avatar Jan 27 '20 21:01 ghost

Thank you for submitting your first issue to this project.

github-actions[bot] avatar Jan 27 '20 21:01 github-actions[bot]

Hi @christopher-walker,

I don't see any specific reason to use the whole context. But it is used all over the controls. For now Field Controls are the only exception. I think it'll be a good improvement to create, say, IContext (or use ootb IBaseComponentContext if we move to the later SPFx version) and use it instead of WebPartContext.

AJIXuMuK avatar Feb 19 '20 21:02 AJIXuMuK

And yes - it's not only about PeoplePicker, but other controls as well.

AJIXuMuK avatar Feb 19 '20 21:02 AJIXuMuK

Nice ... I ended up sort of overriding parts of the people picker object. I guess this will make it more difficult for me to upgrade in the future but I wanted to have a little more control over the objects I was passing in. I ended up creating an interface:

// import { ITask } from '../Common/IObjects'

import { IPeoplePickerUserItem } from @pnp/spfx-controls-react/lib/PeoplePicker

export enum PrincipalType {
    User = 1,
    DistributionList = 2,
    SecurityGroup = 4,
    SharePointGroup = 8
  }

export default  interface IPeopleSearchProvider {

    generateUserPhotoLink(value: string): string;

    getSumOfPrincipalTypes(principalTypes: PrincipalType[]): number;

    getGroupId(groupName: string, siteUrl: string): Promise<number | null>;

    searchPersonByEmailOrLogin(email: string, principalTypes: PrincipalType[], siteUrl: string, groupId: number, ensureUser: boolean): Promise<IPeoplePickerUserItem>;

    searchPeople(query: string, maximumSuggestions: number, principalTypes: PrincipalType[], siteUrl: string, groupId: number, ensureUser: boolean): Promise<IPeoplePickerUserItem[]>;
}

I then have a mock version and a sharepoint version which I pass into my control. Makes it easier for testing.

Anyway thanks for your reply :)

christopher-walker avatar Feb 19 '20 22:02 christopher-walker

Hi, I also had this problem and find a way to mock the whole context. In short I created object with properties needed (site url, sphttpclient ect) and mock the whole sp-http module. You can read in details here: https://mgwdevcom.wordpress.com/2020/03/17/extend-spfx-solution-testability-dealing-with-legacy-code-and-async-components/

mgwojciech avatar Mar 18 '20 07:03 mgwojciech

Thanks for declaring this issue @christopher-walker! I'm also interested in getting an upgrade of the PnP Controls, as I don't want to pass down the whole WebPartContext object...!

michaelmaillot avatar Jun 30 '20 16:06 michaelmaillot

Hi everyone,

Next beta release will include a smaller context called IPeoplePickerContext which requires only necessary props from the BaseComponentContext 🙂

michaelmaillot avatar Mar 11 '24 07:03 michaelmaillot

Hi everyone,

Next beta release will include a smaller context called IPeoplePickerContext which requires only necessary props from the BaseComponentContext 🙂

Nice work Michael 👍

chrisw-n avatar Mar 11 '24 08:03 chrisw-n