adding carousel to attachment modal

[JediWattson]

Details

add a carousel to the attachment modal that navigates through a chat's attachments

Fixed Issues

https://github.com/Expensify/App/issues/7862
PROPOSAL: https://github.com/Expensify/App/issues/7862#issuecomment-1128202430

Tests

iOS / android

1. tap on an image in chat and an attachment modal presents
2. swipe left or right to change the attachment to the next or previous one
3. tap on the modal to make the arrows on the sides appear or disappear
4. tap on an arrow to change the attachment to the next or previous one
5. verify attachment's name are displayed correctly in modal header

web / desktop

1. click an image
2. clicking an arrow will change the attachment to the next or previous one
3. verify that moving the cursor outside the modal will make the arrows disappear
4. verify moving the cursor back into the modal will make the arrows appear
5. verify modal header matches file name, and correct attachment is downloaded

• [x] Verify that no errors appear in the JS console

Offline tests

QA Steps

iOS / android

1. tap on an image in chat and an attachment modal presents
2. swipe left or right to change the attachment to the next or previous one
3. tap on the modal to make the arrows on the sides appear or disappear
4. tap on an arrow to change the attachment to the next or previous one
5. verify attachment's name are displayed correctly in modal header

web / desktop

1. click an image
2. clicking an arrow will change the attachment to the next or previous one
3. verify that moving the cursor outside the modal will make the arrows disappear
4. verify moving the cursor back into the modal will make the arrows appear
5. verify modal header matches file name, and correct attachment is downloaded

• [x ] Verify that no errors appear in the JS console

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for the expected offline behavior in the Offline steps section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • [x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android / native
  • [x] Android / Chrome
  • [x] iOS / native
  • [x] iOS / Safari
  • [x] MacOS / Chrome / Safari
  • [x] MacOS / Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [x] I verified that if a function's arguments changed that all usages have also been updated correctly
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • [x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [x] All JSX used for rendering exists in the render method
  • [x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [x] If any new file was added I verified that:
  • [x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• [x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

https://user-images.githubusercontent.com/6119181/209240380-e1d0d2c8-6d9b-40a9-a13f-d73cd30bf659.mov

Mobile Web - Chrome

https://user-images.githubusercontent.com/6119181/209581657-961beb4a-41c9-40f1-9435-491643434669.mov

Mobile Web - Safari

https://user-images.githubusercontent.com/6119181/209581646-c0f64f3b-81fb-497e-9976-0a6e603a92e9.mov

Desktop

https://user-images.githubusercontent.com/6119181/209240439-beb8e0c8-a46f-418b-acb4-d56e82bf65e4.mov

iOS

https://user-images.githubusercontent.com/6119181/209240861-68fc6624-4096-4186-936b-4291fcfca9b4.mov

Android

https://user-images.githubusercontent.com/6119181/210114328-cbd4e055-382f-4710-92fd-3fb50e3831e7.MOV

[JediWattson]

I set this branch up to get a signed commit in. I'll be looking this comment first and then I'll need to update the videos I made for the updated buttons. Also I think there's about 2 comments that I've responded to that I left unresolved, but the rest I either went with the suggested code or unchanged the code.

[chiragsalian]

Review paused until santhosh get's back from OOO. Adding him as reviewer so that he sees the PR once he's back.

[Santhosh-Sellavel]

@JediWattson From issue description https://github.com/Expensify/App/issues/7862#issue-1146471345

Expected Result:

1. When the member is viewing the most recent attachment in the chat, there's only a left arrow icon on the attachment preview screen to navigate further back. Similarly, when the member is viewing the oldest attachment in the chat, there's only a right arrow to navigate forward.
2. If the member is previewing an attachment that isn't the oldest or latest in the chat history, there will be both left/right arrow icon to navigate "forward" or "back" to the next attachment preview.
3. On desktop/Web: The arrow can be clicked to navigate to the next attachment preview, or the left/right arrow keys on the keyboard can be used
4. On mobile/mWeb: The arrow can be tapped or the member can swipe left/right on the preview to navigate to the next attachment preview depending on the direction
5. On desktop/Web, the arrows are visible on hover. On mobile/mWeb, a single tap on the image hides/shows the arrows.

I believe PR covers only 1 & 2 what above 3, 4, 5.

[JediWattson]

@JediWattson From issue description #7862 (comment)

Expected Result:

1. When the member is viewing the most recent attachment in the chat, there's only a left arrow icon on the attachment preview screen to navigate further back. Similarly, when the member is viewing the oldest attachment in the chat, there's only a right arrow to navigate forward.
2. If the member is previewing an attachment that isn't the oldest or latest in the chat history, there will be both left/right arrow icon to navigate "forward" or "back" to the next attachment preview.
3. On desktop/Web: The arrow can be clicked to navigate to the next attachment preview, or the left/right arrow keys on the keyboard can be used
4. On mobile/mWeb: The arrow can be tapped or the member can swipe left/right on the preview to navigate to the next attachment preview depending on the direction
5. On desktop/Web, the arrows are visible on hover. On mobile/mWeb, a single tap on the image hides/shows the arrows.

I believe PR covers only 1 & 2 what above 3, 4, 5.

for 3 I would propose to add a listener to handle the arrow press. It would be added when the component is mounted:

   componentDidMount() {
        document.addEventListener('keydown', this.handleArrowPress)
    }

    componentWillUnmount() {
        document.removeEventListener('keydown', this.handleArrowPress)
    }

handleArrowPress will check the e.key and call cycleThroughAttachments

    /**
     * Listens for keyboard shortcuts and applies the action
     *
     * @param {Object} e
     */
    handleArrowPress(e) {
        if(e.key === "ArrowLeft" && !this.state.isBackDisabled) {
            this.cycleThroughAttachments(-1);
        }
        if(e.key === "ArrowRight" && !this.state.isForwardDisabled) {
            this.cycleThroughAttachments(1);
        }
    }

4. I see a SwipeableView but I'm unsure how I want to handle the data change because I feel it's best like this:

  <SwipeableView>
    <AttachmentView sourceURL={sourceURL} file={this.state.file} />
  </SwipeableView>
  // ... <AttachmentCarousel 

5. On mobile you want the arrows to initially appear then disappear onPress right? For desktop they would be visible onMouseOver?

[Santhosh-Sellavel]

On mobile you want the arrows to initially appear then disappear onPress right?

5. On desktop/Web, the arrows are visible on hover. On mobile/mWeb, a single tap on the image hides/shows the arrows.

From 5 we can get the answer, we should toggle arrow visibility when tapping on the image.

But I'm not sure whether we should show or hide the arrows initially. @Expensify/design team please help us here.

cc: @shawnborton

[Santhosh-Sellavel]

@JediWattson Sounds good! Please make the suggested changes, let me know when changes are done.

[shawnborton]

For mobile, I think it's okay if they are visible by default and then tapping the image hides them.

[JediWattson]

I just got most of the changes here up. I just need to add the functionality for the arrows to show and not show

[Santhosh-Sellavel]

@JediWattson Any update on this?

[JediWattson]

I'm finishing up the arrows today, and then possibly get into the swiping too. I can add new videos showing this this weekend too.

[Santhosh-Sellavel]

@JediWattson Further update on this?

[github-actions[bot]]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[Santhosh-Sellavel]

@JediWattson You should use only one git account. Do not use multiple accounts. Can you revert & commit changes properly thanks!

[JediWattson]

Will do, I think I've completed the requirements now too. Now it seems I just need to clean that up, and add some videos for swiping and toggling arrows

[JediWattson]

I believe all I have left now is to make some videos showing the arrows show and not showing along with swiping and update the qa instructions

[Santhosh-Sellavel]

Good then,

• Pull changes from the latest main
• Update PR Description & videos

And let us know when PR is ready for review.

[Santhosh-Sellavel]

@JediWattson Bump!

[JediWattson]

@Santhosh-Sellavel I got some new videos up to show the swiping in native apps. A couple things I really can't figure out are: in mobile browsers I'm unable to capture an event to toggle showing the arrows, and in a pdf I couldn't get a similar event so I left the arrows on there

[Santhosh-Sellavel]

A couple things I really can't figure out are: in mobile browsers I'm unable to capture an event to toggle showing the arrows, and in a pdf I couldn't get a similar event so I left the arrows on there

Sorry for the delay, are these both specific to mobile web alone?

[Santhosh-Sellavel]

@JediWattson bump!

[JediWattson]

the arrows not toggling is specific to mobile browsers, but the pdf seems to be an issue for both mobile browsers and native

[JediWattson]

I believe I got this 100% now! @Santhosh-Sellavel

[Santhosh-Sellavel]

Good @JediWattson KIndly update your PR Description along with all other necessary info and updated screen recordings too thanks!

[Santhosh-Sellavel]

@JediWattson Bump!

[JediWattson]

@Santhosh-Sellavel apologies, I just need to update the mobile web videos to show the arrows showing/disappearing and it should be good. I'll try to finish that up in the next couple days

[Santhosh-Sellavel]

@JediWattson bump!

[JediWattson]

@Santhosh-Sellavel I have updated the mobile web videos to reflect the new changes. I should 100% hopefully!

[Santhosh-Sellavel]

Please resolve conflicts & let us know when it is ready for review again thanks!

@JediWattson

[JediWattson]

@Santhosh-Sellavel conflicts are resolved!

[Santhosh-Sellavel]

What about the Other changes I requested in the review, please address that too..

[JediWattson]

@Santhosh-Sellavel do you mean the attachments not working? That should be fixed now

[Santhosh-Sellavel]

Can you check other review comments

[Santhosh-Sellavel]

@JediWattson Sorry for the delay here, I'm not sure why you end up modifying the Swipeable View for onPress. Adding more swipe-related functionality is fine but not okay with adding irrelevant interaction here.

Before moving forward we need to know,

What other options did you explore i.e without modifying this Swipeable component? What's the problem with that method? Why did you think this is the best way to address this?

If you summarise this with enough info, we will get to know more context on this to help us decide on it. We can also take this to slack to get other help on problems you are facing.

Thanks!

cc: @chiragsalian

[JediWattson]

What other options did you explore i.e without modifying this Swipeable component?

I tried to putting Pressable in a place that would take the onTap gesture but not capture any other gestures that make swiping possible. For native mobile I couldn't find a place where that was possible so I added that in Swipeable.

What's the problem with that method?

It doesn't work for mobile browsers and moving Pressable out of there obstructs using Swipeable in native mobile

Why did you think this is the best way to address this?

because it's a use case for mobile web where I can't handle onClick or something that's distinct from mobile and not have it hinder the functionality of mobile.

[Santhosh-Sellavel]

@JediWattson Can you elaborate bit more in details, please include the snippets/commits which we can use to play around with.

mobile browsers and moving Pressable out of there obstructs using Swipeable in native mobile

How did you do that?

[JediWattson]

If I did something like in the below code I'm unable to capture the gestures in Swipable because pressable takes the events now, however it works in mobile web.

<Pressable>
  <Swipable />
</Pressable>

so I used Swipable to handle the tap gesture on native mobile, and then with mobile web the Swipable/index.js containing Pressable now works for web as well

[Santhosh-Sellavel]

It's better we leave Swipeable View untouched. Write a new component Just to handle interactions for AttachmentCarousel, let's separate specific interactions there.

[JediWattson]

I don't get the point of splitting up Swipable if the web based component isn't used to mirror the events found in the native component? I mean why have the index.native.js when that's what it seems to be there for?

[Santhosh-Sellavel]

I don't get the point of splitting up Swipable if the web based component isn't used to mirror the events found in the native component? I mean why have the index.native.js when that's what it seems to be there for?

I'm not getting it, can you ask in other words?

[JediWattson]

why is this component split into two variants for native and web if not to be modified so as to mirror interactions amongst the platforms?

[Santhosh-Sellavel]

why is this component split into two variants for native and web

There is a comment there already, we are supporting swipe only for native apps it seems!

https://github.com/Expensify/App/blob/cc31727073a1f6dfdec49af47689064b598d20bd/src/components/SwipeableView/index.js#L1-L3

[JediWattson]

but now that I needed to add a tap gesture in the SwipeableView that's used for both mobile native and web platforms then it makes sense to modify that to reduce code reuse

[Santhosh-Sellavel]

Swipeable is intended for swipe only actions.

Pressable is intended for tap actions

I don't get it why we want to add a press to Swipeable.

If we need to combine both swipe and tap we should form a new component.

Example take a look at PressableWithSecondaryInteraction which combines longpress & click

[JediWattson]

can I rename Swipable to SwipableWithPressable then? That makes sense, and that comment explains why swiping is only available for iOS/Android

[Santhosh-Sellavel]

@JediWattson

Let's leave Swipeable untouched.

SwipeableWithPressable is not need to be defined we might not use it for other

Sometimes it's okay to reuse code.

Actually, as I said it's better to write a new component Attachment Carousel Action Listener. to define touch/pan/tap listeners split platform-specific only if required.

Props of the component be toggleArrows, nextAttachment or previousAttachment etc.

[JediWattson]

is there an example of this somewhere else in the app so I can understand better what the ask is here?

also SwipeableWithPressable is just the renaming of Swipeable and would solve what you're saying here because it's identifying this component is now pressable too. I mean currently Swipeable is technically incorrect with that pressable gesture being handled now.

[trjExpensify]

@chiragsalian or @marcaaron do either of you have an opinion on this https://github.com/Expensify/App/pull/9279#issuecomment-1257185660 so we can help keep this moving and get across the finish line?

[marcaaron]

I'm not really up to speed on this conversation. Can someone provided a quick summary of what they need help with?

[chiragsalian]

Discussing some of the pending points,

1. also SwipeableWithPressable is just the renaming of Swipeable and would solve what you're saying here because it's identifying this component is now pressable too.

and,

2. can I rename Swipable to SwipableWithPressable then? That makes sense, and that comment explains why swiping is only available for iOS/Android

At this point I think it's better to write multiple Attachment Carousel Action Listener, each having platform specific code. This way one can use swipeable for mobile and another can use pressable for web. I think this is what Santhosh was suggesting too, correct me if I'm wrong. Plus I still don't get why pressable is in swipeable. I don't quite understand why using panresponder means we have to have pressable in swipeable. Maybe if you can explain this more?

3. is there an example of this somewhere else in the app so I can understand better what the ask is here?

Sure does this help, basically, if you look at any of our index.native.js files you can see how we write platform-specific codes. Eg: web, mobile.

[Santhosh-Sellavel]

@marcaaron

PR is not reviewed completely yet this discussion occurred from the initial review itself.

The interaction requirements for this issue,

On desktop/Web: The arrow can be clicked to navigate to the next attachment preview, or the left/right arrow keys on the keyboard can be used On mobile/mWeb: The arrow can be tapped or the member can swipe left/right on the preview to navigate to the next attachment preview depending on the direction On desktop/Web, the arrows are visible on hover. On mobile/mWeb, a single tap on the image hides/shows the arrows.

To achieve this we need to listen,

• Swipe events on native & mWeb platforms (to navigate between view next and previous attachment)
• Tap event on all platforms (to toggle of prev & next arrows)
• hover event on web/desktop(to show arrows)

Now what @JediWattson had done is modified the SwipeableView, to add more swipe functionalities currently, we have only swipeDown action there, also for SwibeableView is available only for native platforms.

• It's okay to add more swipe actions like swipeHorizontal
• But adding onPress to SwipeableView doesn't make any sense, SwipeableView implementation of web/desktop has Pressable wrap.

So I requested changes to modify it in a way, that keeps swipe-related & pressable interactions separate.

@JediWattson says they have a problem keeping it separate as some interactions don't work. Didn't get a clear idea on why even after follow-ups.

So I suggested two options,

• Write a new component that listens to both swipe & pressable, like SwipeableWithPressable to listen to both events
• Then I felt since we do this specifically for this issue, it would be better to write a separate component Attachment Carousel Action Listener and define touch/pan/tap listeners and add props functionalities of the component be like toggleArrows, nextAttachment or previousAttachment etc.

But so far @JediWattson suggesting is renaming SwipeableView to SwipeableWithPressable or modifying SwipeableView

cc: @chiragsalian