Add tap to view full screen image functionality

[thienlnam]

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/142710 and https://github.com/Expensify/Expensify/issues/143523

Tests

1. Upload an image and a PDF
2. Click on them and verify they open correctly

Screenshots

[shawnborton]

Tagging @Expensify/design to follow along for screenshots. Can we get a preview of how this will look/function?

[thienlnam]

Yup! I'll get those added after flushing this out today

[tgolen]

I know this is a WIP, so just adding a few comments. Looks like it's all on the right track 👍

[thienlnam]

Thanks Tim!

As for how this looks/functions, here is a demo on iOS:

The idea is there will be a header on the modal that you can drag down to close / or press the exit button to close

• Still need to update the button icon
• Still need to get gesture handling working

On web I am trying to make it look similar to the way it currently is expensify.com without the profile info so it would hopefully eventually look something like this -

[thienlnam]

FYI: Expanding this PR to also accomodate issue to view PDFs when clicked

[shawnborton]

Hey @thienlnam this looks great, but curious if there are any mockups you are basing your work off of?

[thienlnam]

@shawnborton I've been working from the web modal and looking to have the final look something similar to that. I don't have any mockups for what it would look like on mobile or web with react native. I'd be happy to make adjustments based on any mockups!

[shawnborton]

Yeah, let's try to get you some good mockups. I don't think that should stop your progress from making this functional, but we never mocked up what viewing attachments would look like in the ReactNative world so I think it would be a good idea for us to feel super aligned on how this would look/work across all platforms.

[shawnborton]

Also - why not show the owner of the photo in the modal preview? As in, show the profile picture + name?

[thienlnam]

@shawnborton Sounds good, and my only worry with that right now is that it already takes a little bit for the image to load full screen so it would only be adding more overhead costs until we added some kind of caching but I will work off designs.

[tgolen]

What's the status of this PR? Has it been deprioritized for something else? Looks like it's just collecting conflicts now :D Anything I can do to help push it along?

[thienlnam]

So there has been a discussion around the design of the modal in the main issue, and then @roryabraham is also working on this proposal here so this PR is kind of in a weird spot right now.

This PR is mostly complete, needs the modal to be styled and then figure out PDF viewing on web but if everyone is okay with it then I can continue and push this out and we can refactor it later to be inline with standards that come from the whatsnext discussion

[tgolen]

OK, thanks for the update! Sounds like we should move forward with figuring out the PDF viewing stuff if the design is still be narrowed down. I also think it's fine if we release something that works now, and then improve it later once we have the design discussion settled.

…

On Mon, Oct 26, 2020 at 10:28 AM Jack Nam ***@***.***> wrote: So there has been a discussion around the design of the modal in the main issue <Expensify/Expensify#142710>, and then @roryabraham <https://github.com/roryabraham> is also working on this proposal here <https://expensify.slack.com/archives/CC7NECV4L/p1603489444415900> so this PR is kind of in a weird spot right now. This PR is mostly complete, needs the modal to be styled and then figure out PDF viewing on web but if everyone is okay with it then I can continue and push this out and we can refactor it later to be inline with standards that come from the whatsnext discussion — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#630 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB2A2ICXJGPWL3WT5BLSMWPTFANCNFSM4SKV7D2Q> .

[thienlnam]

Sounds good, I will put this train back on the track then!

[tgolen]

Thanks for those changes. Looks good!

[thienlnam]

Alright, this should be ready to go! @marcaaron is out this week but he tested/approved before he left.

cc @tgolen @roryabraham

[roryabraham]

Overall looks pretty good, just a few suggestions. Sorry for the delayed review - this was my first time fully looking through this PR

[roryabraham]

NAB bind this in the constructor and then get rid of the arrow functions when it's called. I think that's the convention in our react code because it's marginally more performant

[thienlnam]

I binded the function but when I tried removing the arrow functions it just stopped working - I think it might be because I'm passing a parameter in the function as well?

[roryabraham]

LGTM

[tgolen]

The different constants are getting confusing. The more explicit the constant name, the less confusing this will be. I really hate blocking the PR on this though, so I would be OK with merging this PR as-is and then doing an immediate follow-up to clean up these constants.

Here are the things that I find confusing:

• I don't know when to use ngrok or why
• If USE_NGROK=true, then SITE_ROOT and API_ROOT are modified with the ngrok URL. However, if you have to modify the .env file to set USE_NGROK then why not just change SITE_ROOT and API_ROOT instead?
• If NGROCK_URL exists, then it's not clear what happens with EXPENSIFY_SITE_ROOT
• DEFAULT_SITE_ROOT is a very ambiguous name with no meaning
• SITE_ROOT is also ambiguous and easily confused with the other constants
• USE_NGROK is not really necessary because the logic can just check if NGROK_URL exists

Some suggestions:

1. The readme probably needs updated instructions for using ngrok properly (why, when, how, etc.)
2. Get rid of API_ROOT completely. It is only used twice, and those can be switched to be ${SITE_ROOT}api? instead
3. Rename DEFAULT_SITE_ROOT to URL_EXPENSIFY_COM
4. Rename CASH_SITE_ROOT to URL_EXPENSIFY_CASH
5. Rename SITE_ROOT to something else (but I am still unclear what it is used for so I can't suggest a better name right now)
6. Maybe we can add a platform specific .env.native file which would be where the ngrok values are set (so you don't have to constantly be modifying .env when switching between platforms).

[tgolen]

OK, sounds good! I'll be standing by to approve and merge this. Thanks.

…

On Thu, Nov 19, 2020 at 11:32 AM Jack Nam ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/CONFIG.js <#630 (comment)> : > +// Updates the API_ROOT and SITE_ROOT to use the NGROK route if .env flag is enabled +// Otherwise it will use the value inside config.EXPENSIFY_SITE_ROOT +// DEFAULT_SITE_ROOT will always contain the default site root of www.expensify.com or www.expensify.com.dev Yeah I agree I really didn't like adding an extra .env variable of DEFAULT_SITE_ROOT but I really just needed a .env variable that had the actual site root of www.expensify.com.dev to replace the image urls. I do like the rename suggestions though and that will help clear up the confusion. I would be so very happy to merge this PR but I think it would just be easiest to make those changes now and then merge. I'll make these changes quickly and then hopefully we can get this out soon — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#630 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB2HKVVDOVYUT4UPS4DSQVQEBANCNFSM4SKV7D2Q> .

[thienlnam]

@tgolen 🐎 🟢

Should be ready to go now!

[roryabraham]

Okay, I agree the constants ended up being a bit messy. It's partially my fault, because I thought having USE_NGROK would be a convenient way to just "flip a switch" to start using NGROK, but I can see how it complicates things.

Anyways, LGTM so merging 🚀