-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add onClick prop to FormFileUpload #39268
Add onClick prop to FormFileUpload #39268
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @alefesouza! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement!
I haven't tried using the upload()
helper so I'm not sure if it works in our case, but could you try adding a quick UI test for this behavior? Something like it( 'should fire a change event after selecting the same file if event.target.value is nulled on click' )
. The existing test seems to be written in Enzyme but new tests should just be written in @testing-library/react
. (Let us know if you don't have the appetite for this right now, we can address the test separately.)
And if you could add a changelog entry that would be great.
fe65061
to
64f542c
Compare
Definitely not a bug, but an enhancement to allow a common pattern which is to null out the value on file input clicks. Passing your own component via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's it on my end, thanks for your patience through all the hiccups!
We can approve & merge once the remaining tiny things are addressed 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to this PR, the onClick
prop passed to FormFileUpload
would be applied to the Button
, overriding the onClick={ openFileDialog }
handler (as also explicitly explained in the README).
Wouldn't this PR introduce a breaking change?
Even if we agree that it's not a breaking change, we should still update the README and list the onClick
prop explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀 thank you so much @alefesouza for working on this and for the patience in addressing all the feedback!
Let's just wait for @mirka 's blessing as well, since she's been the main reviewer on this PR.
Moved to "Enhancements" subsection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for working on this enhancement, @alefesouza! I have a feeling a lot of people will be benefiting from this 🎉
I just did a quick fixup of the changelog entry, and added a section in the readme. Will merge now.
Congratulations on your first merged pull request, @alefesouza! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
* Add onClick prop to FormFileUpload It will allow to the parent component clear the file input when adding the same file. * Rename onInputFileClick to onClick * Test file change on FormFileUpload component * Add changelog entry * Update changelog entry * Change FormFileUpload test comments * Use user-event@14 recommended setup * Update unit tests * Update unit tests comments * Update unit tests * Fix typos * Update unit tests * Update changelog * Update packages/components/CHANGELOG.md * Fixup changelog Moved to "Enhancements" subsection * Add `onClick` prop to readme Co-authored-by: Marco Ciampini <[email protected]> Co-authored-by: Lena Morita <[email protected]>
Description
Fixes #39267
This PR adds a
onInputFileClick
prop on theFormFileUpload
.Currently when there's an error on the
FormFileUpload
and you need to upload the same file again, it doesn't emit theonChange
event and the user get stuck until choose another file.With this change, we can add
<FormFileUpload onInputFileChange={ (e) => e.target.value }>
on the parent component, this way the<input type="file">
insideFormFileUpload
will empty its file after click, allowing to upload the same file again.I didn't make it runs by default for compatibility purposes.
Testing Instructions
FormFileUpload
component with aonChange
event.onInputFileChange={ (e) => e.target.value }
to the component.onChange
event again.Screenshots
Current behavior, the correct would be show the loading again and re-upload the file again, but as the hidden
<input type="file">
doesn't change its value, it doesn't emit theonChange
event, this PR fixes that.Types of changes
Checklist:
*.native.js
files for terms that need renaming or removal).