Skip to content

💡Added support to ModernTaxonomyPicker in DynamicForm - Fix 1275 [Repo Rescuer Challenge] #1962

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

Conversation

NishkalankBezawada
Copy link
Contributor

@NishkalankBezawada NishkalankBezawada commented Mar 4, 2025

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

What's in this Pull Request?

Added support to ModernTaxonomyPicker control in Dynamic Form. This will be handled by a property useModernTaxonomyPicker

Property Type Required Description
useModernTaxonomyPicker boolean no Specifies if the form should render ModernTaxonomyPicker control for Managed metadata fields. If set to true, Dynamic form will render ModernTaxonomyPicker control. If set to false, Dynamic form will render TaxonomyPicker control. Default is false

By using this property, we ensure that both TaxonomyPicker control and ModernTaxonomyPicker control can be used. This will be automatically defaulted to use TaxonomyPicker control.

Fix1275

Guidance @joelfmrodrigues

Thanks,
Nish

@NishkalankBezawada NishkalankBezawada changed the title InitialCommit - Fix 1275 💡Added support to ModernTaxonomyPicker in DynamicForm - Fix 1275 Mar 4, 2025
@NishkalankBezawada NishkalankBezawada marked this pull request as ready for review March 4, 2025 16:18
@michaelmaillot
Copy link
Collaborator

Hello Nish, thanks for providing this PR.

Here's my two cents about this one, to go further this update. At some point, I was thinking about definitely replacing TaxonomyPicker by the ModernTaxonomyPicker in this control. But for the moment, the ModernTaxonomyPicker relies on the PnPjs v2 @sp/taxonomy package which uses v2.1 endpoint under the hood. But we'll problably update to PnPjs v4 at some point and this will involve to migrate from SP REST call to Graph call (or keep v2.1 REST call but having to overwrite existing calls to make explicit REST API calls instead of using PnPjs taxonomy calls).

From my perspective, with this PR we can let users choose to use the picker they want. But once we plan to update PnPjs version, we'll have to keep in mind that this will involve code base / documentation update regarding the ModernTaxonomyPicker.

@NishkalankBezawada
Copy link
Contributor Author

Hello Nish, thanks for providing this PR.

Here's my two cents about this one, to go further this update. At some point, I was thinking about definitely replacing TaxonomyPicker by the ModernTaxonomyPicker in this control. But for the moment, the ModernTaxonomyPicker relies on the PnPjs v2 @sp/taxonomy package which uses v2.1 endpoint under the hood. But we'll problably update to PnPjs v4 at some point and this will involve to migrate from SP REST call to Graph call (or keep v2.1 REST call but having to overwrite existing calls to make explicit REST API calls instead of using PnPjs taxonomy calls).

From my perspective, with this PR we can let users choose to use the picker they want. But once we plan to update PnPjs version, we'll have to keep in mind that this will involve code base / documentation update regarding the ModernTaxonomyPicker.

Hey @michaelmaillot

I completely agree with your perspective. Allowing users to choose between TaxonomyPicker and ModernTaxonomyPicker for now makes sense, especially considering the future migration to PnPjs v4 and the potential shift to Graph API. To add to it, for the existing components, that uses IPickerTerms in TaxonomyPicker and it wouldnt be wise to change it to use ModernTaxonomyPicker.

In addition to this, I was also thinking to extend ModernTaxonomyPicker to support a more controlled approach, where we provide options to create and delete terms (But this would be in future after discussions). This enhances the flexibility and usability in various scenarios.

Looking forward to any further feedback!

Thanks,
Nish

Copy link
Collaborator

@michaelmaillot michaelmaillot left a comment

Choose a reason for hiding this comment

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

Hey @NishkalankBezawada,

Nice improvment! I've added some feedback, please let me know if you need help.

Would you mind resolving conflicts also?

@NishkalankBezawada
Copy link
Contributor Author

Hey @michaelmaillot

All set for your review.

Thanks,
Nish

@NishkalankBezawada NishkalankBezawada changed the title 💡Added support to ModernTaxonomyPicker in DynamicForm - Fix 1275 💡Added support to ModernTaxonomyPicker in DynamicForm - Fix 1275 [Repo Rescuer Challenge] Mar 15, 2025
Copy link
Collaborator

@michaelmaillot michaelmaillot left a comment

Choose a reason for hiding this comment

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

Hey @NishkalankBezawada,

The error met when typing in the taxonomy field when using ModernTaxonomyPicker should be addressed. I've proposed an update as a suggestion to fix this.

Let me know if you need any assistance.

@NishkalankBezawada
Copy link
Contributor Author

Hey @NishkalankBezawada,

The error met when typing in the taxonomy field when using ModernTaxonomyPicker should be addressed. I've proposed an update as a suggestion to fix this.

Let me know if you need any assistance.

Hello @michaelmaillot

This is now been addressed. All set for your review.

Thanks,
Nish

Copy link
Collaborator

@michaelmaillot michaelmaillot left a comment

Choose a reason for hiding this comment

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

Fix is ok, autocomplete works like a charm👍

Just a few updates to do and we'll be good to go!

@michaelmaillot
Copy link
Collaborator

package.json & package-lock.json are still in the PR 🙃

Could you please remove them?

@NishkalankBezawada
Copy link
Contributor Author

NishkalankBezawada commented Mar 17, 2025

package.json & package-lock.json are still in the PR 🙃

Could you please remove them?

Yepp @michaelmaillot, Its done now.

//Nish

@michaelmaillot michaelmaillot merged commit b484e17 into pnp:dev Mar 17, 2025
1 check passed
@michaelmaillot
Copy link
Collaborator

Merged manually, thanks again for your contribution!

I've updated the PR to state it as an enhancement, not a bug fix according to me.

@michaelmaillot michaelmaillot added this to the 3.21.0 milestone Mar 17, 2025
michaelmaillot added a commit that referenced this pull request Mar 17, 2025
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.

2 participants