FEATURE: use native file picker in composer (PR #13552)

We want to remove completely our custom modal for uploading files in composer and just trigger directly the system file picker.

This PR makes it happen. The fix is pretty simple since we already weren’t using our custom modal on mobile. We just need to start using the same hidden <input type="file"> that we already use on mobile.

It seems to be pretty tricky to test opening a system modal so I haven’t added new tests. We already have tests for file uploading though. We directly trigger jquery-File-Upload plugin hooks in those tests.

GitHub

Can we avoid jQuery here?

Nice cleanup!

Sure thing!

I think this will force a re-translate of the subkeys. Not sure it’s worth the rename.

Overall a good change and a nicer experience than the modal. I saw you are going to pass the list of allowed extensions in another PR. Why not just do it in this same one? I think it is a bit of a confusing experience at the moment because you have no idea what files you are allowed now, leading to:

I think it would be better if we got rid of jQuery here too!

Why not just do it in this same one?

It’s better to avoid doing several things in one PR. Small PRs are easier to follow, it’s not only important now when we’re reviewing this but also will be important in the future when we’ll be revisiting the history. But more importantly - this change can cause issues on specific platforms and browsers. The smaller the change - the less a chance I miss something when testing this or reviewers miss some important detail.

There are two tasks here:

  1. Get rid of the custom modal
  2. Start passing supported file extensions to the system modal (we weren’t doing it before)

By the way, these tasks can even be done in the opposite order.

I think it is a bit of a confusing experience at the moment

Fair point, but at the same time I think it’ll be confusing only for a tiny amount of users. Most of them just always upload allowed types of files (images and so on). If someone tries to upload an unsupported file he would get the modal with a clear explanation of what’s wrong.

I think it’s more like an annoying experience, not a confusing one. It’s not like “I don’t understand what’s going on”, it’s more like “Why didn’t you tell me earlier that this type of file is unsupported?”

So I’d like us to have every step in a separate PR, the second PR is going to be tiny and will follow soon. @martin-brennan what do you think?

@AndrewPrigorshnev yes I understand the concept of smaller PRs with a single responsibility I’m not arguing against that. I just thought the change to add the allowed file types would be quite small and it’s related to the use of the native file picker anyway. I think that it is a complete change to have that all in one PR.

I think it’s more like an annoying experience, not a confusing one. It’s not like “I don’t understand what’s going on”, it’s more like “Why didn’t you tell me earlier that this type of file is unsupported?”

To me these two things are equally as bad. We don’t want our users to be annoyed or confused.

If you think that the accepted file change is distinct enough to go into a separate PR it’s not a hill I will die on. I just think we should try avoid confusion/annoyance for the end user whenever possible. I’ll approve once the other jQuery click event is changed :slight_smile:

I’ve removed jQuery here.

I think you are right, better to avoid re-translating, I’ve reverted renaming.

To me these two things are equally as bad. We don’t want our users to be annoyed or confused.

@martin-brennan I agree with this! Still feel uncomfortable about doing both changes in one step in this specific case though.

It would definitely worth doing this in one step if we didn’t have that modal with an explanation of which extensions users should use. With the modal everything is not so bad.

I’ll approve once the other jQuery click event is changed :slight_smile:

Thank you :slight_smile: I’ve made that change.