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/#958 Add selected items for groups #962

Merged

Conversation

a1exymoroz
Copy link
Contributor

Q A
Bug fix? [ ]
New feature? [ x ]
New sample? [ ]
Related issues? #958

What's in this Pull Request?

  • Description was fixed
  • Security group secondary text returns email of group

@gautamdsheth
Copy link
Contributor

@a1exymoroz - This is a breaking change. So, could you please modify the PR to make it so that defaultSelectedUsers and defaultSelectedItems are both supported ?

If defaultSelectedUsers are specified, just append it to defaultSelectedItems array internally.
Once we move to maybe v4, we can remove the defaultSelectedUsers property.

@a1exymoroz
Copy link
Contributor Author

a1exymoroz commented Jul 19, 2021

@a1exymoroz - This is a breaking change. So, could you please modify the PR to make it so that defaultSelectedUsers and defaultSelectedItems are both supported ?

If defaultSelectedUsers are specified, just append it to defaultSelectedItems array internally.
Once we move to maybe v4, we can remove the defaultSelectedUsers property.

Ok. I'll update the PR

@a1exymoroz a1exymoroz force-pushed the feat/#958-PEOPLE-PICKER-DEFAULT-SELECTED-ITEMS branch from a977ca5 to 3e2fb76 Compare July 23, 2021 18:39
@AJIXuMuK
Copy link
Collaborator

Sorry for not looking into this PR earlier.

I don't think we need to provide some new property. Why don't just use defaultSelectedUsers as defaultSelectedItems?
Yes, the name is not the best one and we can change it in the next major version. But 2 separate properties will bring more confusion.

@a1exymoroz
Copy link
Contributor Author

Sorry for not looking into this PR earlier.

I don't think we need to provide some new property. Why don't just use defaultSelectedUsers as defaultSelectedItems?
Yes, the name is not the best one and we can change it in the next major version. But 2 separate properties will bring more confusion.

I can revert to the first commit

Actually I need only one changes in

image

Without this changes I can't set default selected items for security group

@AJIXuMuK
Copy link
Collaborator

@a1exymoroz - yes, I agree that this change is needed (maybe with modification to check if element.EntityData is defined). But I think additional property brings confusion.

@a1exymoroz a1exymoroz force-pushed the feat/#958-PEOPLE-PICKER-DEFAULT-SELECTED-ITEMS branch from 3e2fb76 to 26849ba Compare July 29, 2021 14:48
@a1exymoroz a1exymoroz force-pushed the feat/#958-PEOPLE-PICKER-DEFAULT-SELECTED-ITEMS branch from 26849ba to 5bdf0ad Compare July 29, 2021 14:52
@AJIXuMuK AJIXuMuK merged commit 53d6b8b into pnp:dev Aug 6, 2021
@AJIXuMuK
Copy link
Collaborator

AJIXuMuK commented Aug 6, 2021

Thank you @a1exymoroz for the change. And sorry for back-and-forth communication!

@AJIXuMuK AJIXuMuK added this to the 3.3.0 milestone Aug 6, 2021
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.

3 participants