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

PeoplePicker Control WebPart Context #473

Closed
christopher-walker opened this issue Jan 27, 2020 · 9 comments
Closed

PeoplePicker Control WebPart Context #473

christopher-walker opened this issue Jan 27, 2020 · 9 comments
Labels
status:fixed-next-drop Issue will be fixed in upcoming release. type:question
Milestone

Comments

@christopher-walker
Copy link

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

@ghost
Copy link

ghost commented Jan 27, 2020

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

@ghost ghost added the Needs: Triage 🔍 label Jan 27, 2020
@github-actions
Copy link

Thank you for submitting your first issue to this project.

@AJIXuMuK
Copy link
Collaborator

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
Copy link
Collaborator

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

@christopher-walker
Copy link
Author

christopher-walker commented Feb 19, 2020

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 :)

@mgwojciech
Copy link

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/

@michaelmaillot
Copy link
Collaborator

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 michaelmaillot added the status:fixed-next-drop Issue will be fixed in upcoming release. label Mar 11, 2024
@michaelmaillot michaelmaillot added this to the 3.18.0 milestone Mar 11, 2024
@michaelmaillot
Copy link
Collaborator

Hi everyone,

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

@chrisw-n
Copy link

Hi everyone,

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

Nice work Michael 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:fixed-next-drop Issue will be fixed in upcoming release. type:question
Projects
None yet
Development

No branches or pull requests

5 participants