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

Add onClick prop to FormFileUpload #39268

Merged

Conversation

alefesouza
Copy link
Contributor

@alefesouza alefesouza commented Mar 7, 2022

Description

Fixes #39267

This PR adds a onInputFileClick prop on the FormFileUpload.

Currently when there's an error on the FormFileUpload and you need to upload the same file again, it doesn't emit the onChange 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"> inside FormFileUpload 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

  1. Go to a page that has the FormFileUpload component with a onChange event.
  2. Add onInputFileChange={ (e) => e.target.value } to the component.
  3. Choose any file.
  4. Choose the same file again, it should emit the onChange event again.

Screenshots

Untitled

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 the onChange event, this PR fixes that.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 7, 2022
@github-actions
Copy link

github-actions bot commented Mar 7, 2022

👋 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.

@Mamaduka Mamaduka added [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. labels Mar 8, 2022
@mirka mirka requested review from mirka and ciampo March 9, 2022 11:11
Copy link
Member

@mirka mirka left a 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.

packages/components/src/form-file-upload/index.js Outdated Show resolved Hide resolved
@mirka
Copy link
Member

mirka commented Mar 16, 2022

But I don't think it's a bug that needs to be fixed.

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 render prop won't work because to null out the value you need a reference to the <input>, not the <button>. Because this is a relatively common use case with an established solution, I do think it would be nice to have FormFileUpload support that simple solution (instead of providing some other kind of ref-passing API).

Copy link
Member

@mirka mirka left a 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 🙏

packages/components/src/form-file-upload/test/index.js Outdated Show resolved Hide resolved
packages/components/src/form-file-upload/test/index.js Outdated Show resolved Hide resolved
packages/components/src/form-file-upload/test/index.js Outdated Show resolved Hide resolved
packages/components/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ciampo ciampo left a 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.

Copy link
Contributor

@ciampo ciampo left a 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.

Copy link
Member

@mirka mirka left a 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.

@mirka mirka merged commit 8699e17 into WordPress:trunk Mar 23, 2022
@github-actions
Copy link

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!

@github-actions github-actions bot added this to the Gutenberg 12.9 milestone Mar 23, 2022
jostnes pushed a commit to jostnes/gutenberg that referenced this pull request Mar 23, 2022
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FormFileUpload does not allow upload the same file after error
5 participants