Skip to content

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented Aug 3, 2021

ComposeMenu: Offer file picker on iOS, not just Android

I've just tested on my iPhone (13 Pro, running iOS 16.1), and I was
able to upload a file just fine after this change. That was easy!

Fixes: #4586

cc @alya for the UX change

(edited for revision on 2023-01-10; screenshots in a later message)

@chrisbobbe chrisbobbe requested review from gnprice and WesleyAC August 3, 2021 16:35
@chrisbobbe chrisbobbe added a-iOS a-compose/send Compose box, autocomplete, camera/upload, outbox, sending P1 high-priority labels Aug 3, 2021
@chrisbobbe
Copy link
Contributor Author

Inheriting P1 from the issue.

@gnprice
Copy link
Member

gnprice commented Sep 16, 2021

(Chat discussion of this / #4586 was here.)

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jan 10, 2023

Revision pushed, and I'm re-adding the P1 label given the renewed interest in this feature.

Here's a screenshot of the feature working (except that the inserted text isn't visible in the compose box; that'll be caused by the multiline bug first described in #5291 and to be fixed in PR #5635):

Jan-10-2023 15-00-07

@gnprice
Copy link
Member

gnprice commented Jan 11, 2023

Thanks for pushing on this! The code looks great.

(except that the inserted text isn't visible in the compose box; that'll be caused by the multiline bug first described in #5291 and to be fixed in PR #5635):

Hmm, that seems like a pretty bad symptom -- it's likely to make the user think it didn't work at all. I'll go take a look at #5635 now and see if we can merge that first.

@gnprice
Copy link
Member

gnprice commented Jan 11, 2023

OK, just reviewed #5635. Not quite ready to merge, but I think we'll be able to do so soon. So let's let this PR just hang out for a bit, and we can merge it once that's in.

@gnprice
Copy link
Member

gnprice commented Jan 11, 2023

OK, and #5635 is now merged. Would you rebase this PR and resolve the conflicts?

Because it's been a while since we upgraded it, and because
upgrading came up in a not-so-recent discussion of opting into the
iOS side of this library, in zulip#4586.

The release notes announce some breaking changes, but those
shouldn't affect us:
- A few changes to `fileCopyUri`, but we don't use that
- A change to `pick`, which we don't use (we use `pickMultiple`)
- Desupport Android <5 (which we've already done)

Changelog:
  https://github.com/rnmods/react-native-document-picker/releases
I've just tested on my iPhone (13 Pro, running iOS 16.1), and I was
able to upload a file just fine after this change. That was easy!

Fixes: zulip#4586
I don't get any warnings from Jest with this usual style of import.
It looks like r-n-document-picker handled that in
  react-native-documents/document-picker#474
, first released in v6.0.5. We upgraded past that version a few
commits ago.
@chrisbobbe
Copy link
Contributor Author

Sure, revision pushed. Here's a screen recording:

Jan-11-2023 11-31-43

@gnprice
Copy link
Member

gnprice commented Jan 11, 2023

Thanks! Looks good; merging.

@gnprice gnprice merged commit 1c891b9 into zulip:main Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-compose/send Compose box, autocomplete, camera/upload, outbox, sending a-iOS P1 high-priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload a file from compose box, on iOS
2 participants