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

[Native Share] Implement Native Share for Android #48789

Open
grgia opened this issue Sep 9, 2024 · 45 comments
Open

[Native Share] Implement Native Share for Android #48789

grgia opened this issue Sep 9, 2024 · 45 comments
Assignees
Labels

Comments

@grgia
Copy link
Contributor

grgia commented Sep 9, 2024

Description

This is a tracking issue for the Android implementation of the Native Share project. This section of the project will be handled separately but merged alongside the IOS issue.

Project

https://docs.google.com/document/d/14C1VifxvIXeyLAf0XstbC-wvyAb5SQZ0AOw0-jLRyZs/edit

Tracking

#36613

Issue OwnerCurrent Issue Owner: @filip-solecki
@grgia grgia added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 9, 2024
@grgia grgia self-assigned this Sep 9, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@BrtqKr
Copy link
Contributor

BrtqKr commented Sep 9, 2024

Hey, I'm from SWM, I'd like to take care of this issue.

Copy link

melvin-bot bot commented Sep 12, 2024

@JmillsExpensify, @grgia, @BrtqKr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Sep 16, 2024

@JmillsExpensify, @grgia, @BrtqKr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@JmillsExpensify
Copy link

Going through the process.

Copy link

melvin-bot bot commented Sep 18, 2024

@JmillsExpensify, @grgia, @BrtqKr 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

Copy link

melvin-bot bot commented Sep 20, 2024

@JmillsExpensify, @grgia, @BrtqKr 10 days overdue. I'm getting more depressed than Marvin.

@grgia
Copy link
Contributor Author

grgia commented Sep 23, 2024

Bumped for update / steps for unblocking permissions

@BrtqKr
Copy link
Contributor

BrtqKr commented Sep 23, 2024

I am focusing on split navigation atm. I'll try to complete the flow whenever I'm done with the nav.

@melvin-bot melvin-bot bot removed the Overdue label Sep 23, 2024
Copy link

melvin-bot bot commented Sep 26, 2024

@JmillsExpensify, @grgia, @BrtqKr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Sep 26, 2024
@BrtqKr
Copy link
Contributor

BrtqKr commented Sep 30, 2024

We've split the work with @filip-solecki and he's taking care of the android atm. We're implementing both platforms in parallel

@melvin-bot melvin-bot bot removed the Overdue label Sep 30, 2024
@filip-solecki
Copy link
Contributor

Hi! Yes, I am working on this issue right now

@grgia
Copy link
Contributor Author

grgia commented Sep 30, 2024

Thanks @filip-solecki and @BrtqKr !

@grgia
Copy link
Contributor Author

grgia commented Oct 1, 2024

Hey, @filip-solecki, do you have a draft PR up for this?

@filip-solecki
Copy link
Contributor

Yes, it was created a long time ago. I've updated it and working on it! Here it is

@melvin-bot melvin-bot bot removed the Overdue label Oct 22, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

@JmillsExpensify, @grgia, @BrtqKr, @filip-solecki Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Oct 25, 2024
@grgia
Copy link
Contributor Author

grgia commented Oct 29, 2024

@filip-solecki mind posting a quick update here as well?

Copy link

melvin-bot bot commented Oct 29, 2024

@JmillsExpensify, @grgia, @BrtqKr, @filip-solecki 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@filip-solecki
Copy link
Contributor

Hi! @Guccio163 is currently working on the shared file preview and remaining JS-related issues, while I am working on adding support for text sharing.

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2024
Copy link

melvin-bot bot commented Nov 4, 2024

@JmillsExpensify, @grgia, @BrtqKr, @filip-solecki Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Nov 4, 2024
@grgia
Copy link
Contributor Author

grgia commented Nov 6, 2024

@filip-solecki could you give me an update on when you think we could get a draft PR up for android as well?

@JmillsExpensify JmillsExpensify added Monthly KSv2 and removed Daily KSv2 labels Nov 6, 2024
@melvin-bot melvin-bot bot removed the Overdue label Nov 6, 2024
@filip-solecki
Copy link
Contributor

Hi! Same as for iOS as this will be one PR for both platforms

@garrettmknight garrettmknight moved this to Future Cohort Project in [#whatsnext] #expense Nov 11, 2024
@Guccio163
Copy link
Contributor

Hi @grgia, how do you think we should implement attachment preview section (visible with the donut below) - should it always crop to the predefined dimensions, or maybe scale to fit the whole attachment? It's quite different from Submit flow, because we have to count pdf and videos in, which display in different dynamics than Image in AttachmentView (pdfs can be scrolled and always scale to full width, videos can't be cropped).

Screenshot 2024-11-21 at 08 10 45

@grgia
Copy link
Contributor Author

grgia commented Nov 22, 2024

@Expensify/design could you help out with this question

@dannymcclain
Copy link
Contributor

Is it possible to open the attachment modal from this share screen? Like if I tap on that donut image, do I get the attachment modal showing the whole image?

@grgia
Copy link
Contributor Author

grgia commented Nov 26, 2024

@Guccio163 @filip-solecki could you send an update on this? Also did you get a chance to check out this comment

@filip-solecki
Copy link
Contributor

Sure, Android part is done, right now we are working on polishes for iOS as it will be included in one PR

@filip-solecki
Copy link
Contributor

Is it possible to open the attachment modal from this share screen? Like if I tap on that donut image, do I get the attachment modal showing the whole image?

Sure it is

@shawnborton
Copy link
Contributor

If you can open up the whole attachment in a preview modal, then maybe the easiest thing to do is reuse the attachment box we have when you go to submit an expense?

@Guccio163
Copy link
Contributor

I'll look closely into it, but from what I've checked it's not that straightforward, because Submit allows only images and right now the biggest problem is making it unified for images, pdfs and videos. Because we are thinking about allowing user to open fullscreen AttachmentView after clicking on the miniature, last friday I came up with an idea to not use rescaled AttachmentView in a thumbnail (which is tricky), but rather try to scale thumbnails used in chats for these types of media.
Screenshot 2024-11-26 at 13 35 42
WDYT @shawnborton ?

@shawnborton
Copy link
Contributor

but rather try to scale thumbnails used in chats for these types of media.

That works for me, I agree we need to support multiple media formats here on the share page so however you think is best to accomplish that works for me.

@dannymcclain
Copy link
Contributor

Yup that works for me too! That's kinda where I was going when I asked about the attachment modal. I think since you can "open" the attachment, it's much less important how we display the thumbnail.

however you think is best to accomplish that works for me

Agree with this ☝️

@Guccio163
Copy link
Contributor

Guccio163 commented Dec 3, 2024

Hi, I'm coming with an update on this issue!

We are successfully displaying thumbnails for Images and Videos, though I'm not sure if we should align them to center of left, what do you think?

Screenshot 2024-12-03 at 14 39 31

Also, we're using DefaultAttachmentView for any file other than image and video:

Screenshot 2024-12-03 at 14 30 42

That leaves us with PDFs: When PDF thumbnails are being displayed f.ex. in chats, they use .jpg thumbnail images generated specifically on backend. When using share-extension, whole data is sourced from files locally on device, so the only way to properly show PDFs thumbnail is to:

  1. extract first page of the file on device (platform specific)
  2. convert it into .jpg file
    3 a. save file and send it's localisation
    or
    3 b. send file as a blob

Both ways are potentially time consuming, so right now @filip-solecki and I propose to use DefaultAttachmentView for PDF just like for all other files and after user clicks on it, show the file in AttachmentModal (or not). That of course is available, because AttachmentModal uses whole files to display them, but could be misleading to users that some files shown as DefaultAttachmentView are able to be viewed in modal and some aren't.

Let me know what you think; We still have to fix a couple of new cases introduced in share-extension, but I feel we are really close to creating the draft soon 👌

@shawnborton
Copy link
Contributor

I think left-aligned makes the most sense, cc @Expensify/design for thoughts though!

@dannymcclain
Copy link
Contributor

I'm down with left-aligned.

And re: PDFs, I think it's fine to display them using the "other file" preview style.

could be misleading to users that some files shown as DefaultAttachmentView are able to be viewed in modal and some aren't

While this is totally true, I personally don't think it's a huge deal. I don't think clicking on a PDF or text file to look at it is going to be common here, so I personally don't think it will lead to much, if any, confusion. If we're REALLY concerned about it, I even think it would be ok to just disable the AttachmentModal for PDFs in this situation. Just my 2 cents!

@dubielzyk-expensify
Copy link
Contributor

Left aligned works for me too 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Future Cohort Project
Development

No branches or pull requests

8 participants