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

Added file handling in DynamicForm #1625

Merged
merged 20 commits into from
Nov 27, 2023

Conversation

GuidoZam
Copy link
Contributor

Q A
Bug fix? [ ]
New feature? [x ]
New sample? [ ]

What's in this Pull Request?

Added the possibility to choose the file to be uploaded to the document library.

IRRDC and others added 2 commits August 4, 2023 11:44
Dynamic Form accessed TaxonomyFieldTypeMulti without considering sub-array "results".
src/loc/pt-pt.ts Outdated Show resolved Hide resolved
src/loc/pt-pt.ts Outdated Show resolved Hide resolved
src/loc/pt-pt.ts Outdated Show resolved Hide resolved
@GuidoZam GuidoZam marked this pull request as draft August 17, 2023 07:52
@GuidoZam GuidoZam marked this pull request as ready for review August 18, 2023 08:51
@michaelmaillot michaelmaillot self-assigned this Sep 19, 2023
src/loc/fr-ca.ts Outdated Show resolved Hide resolved
src/loc/fr-ca.ts Outdated Show resolved Hide resolved
src/loc/fr-ca.ts Outdated Show resolved Hide resolved
src/loc/fr-fr.ts Outdated Show resolved Hide resolved
src/loc/fr-fr.ts Outdated Show resolved Hide resolved
src/loc/fr-fr.ts Outdated Show resolved Hide resolved
@michaelmaillot
Copy link
Collaborator

Hi @GuidoZam,

Thanks for this enhancement!

After going through the code and the new props spec, I want to be sure of what you want to cover.

If I read you PR comment, you want to provide a feature that allows to upload a file to a document library.

But if I have a look at the enableFileSelection prop definition here, it says that you can add a file to a list item.

I tried with both approaches and it seems that it's not working with the list item context.

Here's what I suggest:

  • If you want to only cover document libraries, it would be better to clarify it in the prop definition
    • In that case, maybe adding a couple of checks in order to display the file selection button based on the BaseTemplate property for example
  • If you want to cover also lists, it has to be covered in the code
    • You can adapt the queries to do (add / update list item + add list attachment) from the BaseTemplate property also

In all the cases, can you upade the control's documentation page to reflect the enhancement? You could add a screen / video that covers this new feature if you want to 🙂

@GuidoZam
Copy link
Contributor Author

Thanks @michaelmaillot for the reviews and the feedbacks.
I want to cover the case of adding the file when creating a new list item inside a document library, I'll try to enhance the prop description and to also add some checks.

@AJIXuMuK
Copy link
Collaborator

AJIXuMuK commented Oct 8, 2023

@GuidoZam - seems like I still can enable file upload for standard lists (not doc libs)

@GuidoZam
Copy link
Contributor Author

@AJIXuMuK I've added a check using the contentTypeId property of the dynamicForm control, that should be enough to restrict the usage only to document libraries.

@michaelmaillot
Copy link
Collaborator

Hi @GuidoZam, thanks for the update!

I've made some comments as a review.

Sorry to insist but could you please update the DynamicForm's documentation page in order to reflect the new props added in this PR? Feel free to insert screens / gifs or not.

@GuidoZam
Copy link
Contributor Author

GuidoZam commented Nov 1, 2023

Hi @michaelmaillot, sorry for the delay, I'll have a look at your reviews and update the documentation in the next days.

@GuidoZam
Copy link
Contributor Author

Hi @michaelmaillot I've made some improvements, can you please check if now it's all good?

@michaelmaillot
Copy link
Collaborator

michaelmaillot commented Nov 15, 2023

Hi @GuidoZam,

Sounds good to me! Can you just resolve the remaining conflict before I merge your PR please?

Edit: I just saw that there was a tiny update to do on the DynamicForm doc page, which I've pointed out.

@GuidoZam
Copy link
Contributor Author

Hi @michaelmaillot, I've resolved the remaining conflict.
For the update regarding the doc page I cannot see anything, am I missing something?

@michaelmaillot
Copy link
Collaborator

I've mentioned you in the review comment here

@GuidoZam
Copy link
Contributor Author

@michaelmaillot sorry but I can't see any comment there, if I click on the link you inserted it just select the row 62 of the DynamicForm.md file.

src/controls/dynamicForm/DynamicForm.tsx Outdated Show resolved Hide resolved
docs/documentation/docs/controls/DynamicForm.md Outdated Show resolved Hide resolved
@michaelmaillot
Copy link
Collaborator

@michaelmaillot sorry but I can't see any comment there, if I click on the link you inserted it just select the row 62 of the DynamicForm.md file.

Nevermind, forgot to submit my review.........🤦‍♂️

Sorry about that.

@GuidoZam
Copy link
Contributor Author

@michaelmaillot sorry but I can't see any comment there, if I click on the link you inserted it just select the row 62 of the DynamicForm.md file.

Nevermind, forgot to submit my review.........🤦‍♂️

Sorry about that.

No problem, I've resolved the change request.

@michaelmaillot michaelmaillot merged commit 2648fbb into pnp:dev Nov 27, 2023
1 check failed
@michaelmaillot
Copy link
Collaborator

PR merged, thank you for your patience and your work @GuidoZam!

@GuidoZam GuidoZam deleted the dynamicform-add-file branch November 27, 2023 08:27
t0mgerman pushed a commit to t0mgerman/sp-dev-fx-controls-react that referenced this pull request Dec 1, 2023
@AJIXuMuK AJIXuMuK added this to the 3.17.0 milestone Dec 2, 2023
@joelfmrodrigues joelfmrodrigues mentioned this pull request Feb 5, 2024
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.

6 participants