[HOLD] Chat / Cycle attachments when the user presses the left/right keys

[jboniface]

If you haven’t already, check out our contributing guidelines for onboarding!

---

Platform - version:
All platforms, 280

Action Performed (reproducible steps):

1. Click the paperclip icon
2. Select an image to upload
3. Upload the file
4. Repeat steps 1-3 with another image
5. Click into one of the images to expand the attachment modal
6. Press left/right on your keyboard
7. Observe that the same attachment stays on the screen -- the attachments are not cycled

Expected Result: The attachments present in the chat should cycle when the user is viewing the modal and presses the left/right arrows on their keyboard

Actual Result: The attachments do not cycle

Notes/Photos/Videos:

View the job on Upwork here.

[laurenreidexpensify]

Invited two contributors to accept offer on Upwork

[ThatOneAwkwardGuy]

Hi, I've looked through the code and have a few methods of implementation. Both include me writing a function that gets the next attachment from the sorted actions array.

How I pass that function down to the attachment modal has 2 possibilities. Either I pass the function down through the action object, the HTML component and into the attachment modal or I write a react context and attempt to bypass all the unnecessary prop definitions.

[ThatOneAwkwardGuy]

Another update. Seems the react-native-render-html module doesn't work when passing props to renderers for some reason. the passProps value is always undefined.

[ThatOneAwkwardGuy]

Could i ask why "react-native-render-html": "^6.0.0-alpha.10", is being used? it seems to be missing a lot of features of the current v5 code.

[tgolen]

Hi, @ThatOneAwkwardGuy you can see more of the context around that upgrade in this PR: https://github.com/Expensify/Expensify.cash/pull/882 I hope that helps!

[tgolen]

At this point, it's not, no. We haven't developed best practices for using hooks yet, and until we do that we would like to avoid using them.

I'm reading up more about this issue so that I'm more familiar with the proposal being discussed. It seems that the modal itself needs access to the report actions array (specifically the actions that are attachments isAttachment === true). The attachment modal can access those actions from Onyx the same as is being done in the header here. It looks like all that is needed is to pass the reportID to the AttachmentModal inside ReportActionCompose.js and then add a new key to withOnyx() in AttachmentModal.js.

That will allow the modal to access all the report actions without needing to use hooks or to do a lot of prop-drilling. Does that help at all?

[ThatOneAwkwardGuy]

At this point, it's not, no. We haven't developed best practices for using hooks yet, and until we do that we would like to avoid using them.

I'm reading up more about this issue so that I'm more familiar with the proposal being discussed. It seems that the modal itself needs access to the report actions array (specifically the actions that are attachments isAttachment === true). The attachment modal can access those actions from Onyx the same as is being done in the header here. It looks like all that is needed is to pass the reportID to the AttachmentModal inside ReportActionCompose.js and then add a new key to withOnyx() in AttachmentModal.js.

That will allow the modal to access all the report actions without needing to use hooks or to do a lot of prop-drilling. Does that help at all?

Yes it really does, I was going through all m ideas on getting the actions array to the Modal

[ThatOneAwkwardGuy]

After playing around with the code more, there is a problem with the structure of the components. I've written a function that gets the next attachment and cycles around if there aren't any more at the end of the list.

However, the problem is that the attachments provide HTML which is passed to renderHTML. since any change in the fragment (i.e swapping it out for the next one) will cause a re-render of the renderHTML component there will also be a rerender of the AttachmentModal which is inside the renderHTML component. I'll attempt to refactor the code so that there is a singular modal for rendering attachments and it is outside of the entire flatlist of actions.

[ThatOneAwkwardGuy]

Completed my solution, just cleaning up the code. Hopefully, you'll find it acceptable. Had to move the Attachment Modal to ReportActionsView and pass a function to the TouchableOpacity wrapping the attachments you can click to then launch the modal with the necessary data to display on the modal. Happy to answer any questions once I have the code pushed

[HasaanA16]

Hi, So I have implemented a solution as follows: Made a functional component and beside this used 2 modules i.e. react native crop image picker and react native carousel and call this functional component in the developed screen to select images and review them

[Jag96]

@HasaanA16 I've got a few questions about your proposed solution:

1. How will the new functional component interact with the currently existing AttachmentModal component?
2. What functionality from react native crop image picker do you plan on using? I'm not sure we need that here, so I'd like some more details about how it fits into your solution
3. Why should we use a carousel instead of updating the AttachmentModal?

[HasaanA16]

The functional component I propose allows the user to click on the paper click icon where he will see the option of selecting image from the device. Copy the code from my proposed solution and update your component with it.

From react-native-image-crop picker I used the functionality to select multiple images and save their record too as other modules doesn't provide this functionality.

I intended to use the carousel but I didn't. I just used a ScrollList which will show all the images we have selected to preview them when the user swipe right or left

[Jag96]

From react-native-image-crop picker I used the functionality to select multiple images and save their record too as other modules doesn't provide this functionality.

It looks like this issue is for enabling a user to cycle through already uploaded attachments via the existing AttachmentModal, so I don't think we need to add another image picker (we're already using an AttachmentPicker).

I just used a ScrollList which will show all the images we have selected to preview them when the user swipe right or left

This sounds like you're updating the attachment picker component to be able to scroll through just the attachments that were selected by an image picker after clicking the paper clip icon. Just to confirm, will your ScrollList also enable users to click on an existing attachment in the chat, and then preview all other already existing attachments in the chat by using the arrow keys on web?

[HasaanA16]

No, this ScrollList allows the user to preview and navigate through images before sending them. If you require I can update this component with a modal where user can preview while navigating already sent attachments.

[Jag96]

If you require I can update this component with a modal where user can preview while navigating already sent attachments.

Yes, please provide a proposal for allowing the user to navigate between already existing attachments on the chat via the AttachmentModal. Updating the attachment picker to allow for multiple file uploads isn't necessary for this issue.

[HasaanA16]

sure.! I am on it, will ping you in day or two with the changes that you require and hopefully you will like it.

[jboniface]

I talked this over with @chiragsalian and I think he makes a good point that the carousel (i.e. cycling) behavior is unnecessary in this context, as there is unlikely to be a context where you need to loop back to the first image you ever uploaded. i think we can probably cut this behavior.

[HasaanA16]

As you say so, its your call. So should I not make any changes to the code ? Also let me know if there is something I can do here.

[jboniface]

To clarify, I just mean the part of the behavior where the attachments loop -- instead, when you are at the latest attachment in the chat, pressing right on your keyboard should not loop you to the first image you uploaded.

[marcaaron]

Leaving a quick summary of where we're at with suggested next steps for this issue…

This issue didn’t really have clear steps on how to complete it so there was a lot of back and forth on the best way to implement. Ultimately, we didn't feel that we had the best possible solution. But we do have a better idea of the scope of this issue and what kinds of problems we will face to solve it.

Problems

1. We are not able to pass props to a react-native-render-html custom renderer. We had this ability but for some reason lost it when switching to the latest beta. We should first restore this ability before picking up this issue again. Not being able to pass props led to solutions that were more complicated than necessary.
2. The AttachmentModal should probably be refactored before we ask anyone to work on this specific issue again.
3. We should be more specific about how we want this implemented.

Suggested course of action:

1. Reach out to the react-native-render-html maintainers and see if we can get the props to be passed to the custom renderer for the beta version we are on. If that's not possible, we should stop using a custom renderer and instead detect when we have an attachment and skip the RenderHTML component.
2. Refactor the Modal for displaying attachments and the modal for uploading attachments into separate components.
3. We should then pass an array of attachments to the AttachmentModal and an index to each attachment preview. This way we only need to track and set an "active index" for determining which attachment to display in the modal.

[HasaanA16]

I've used react-native-image-crop module which will allow user to select single or multiple attachment.. I've than used react-native-image-view modal which will be prompt if a user tap on any image I've added the functionality of selecting the current image on which it is tapped and than scroll left and right to the other images already present in the chat. I've not used carousel and it is not an infinite loop if user scrolls till the first image he can't scroll left more to see images.

Also I can share video, if you guys want to have a look at it.

[marcaaron]

Hey, @HasaanA16 I think it would be best if we paused working on this until we resolve my comment above.

Anyways, I'm confused about your proposal since you are describing what we already have, but mentioned multiple attachments which should not be supported.

I've added the functionality of selecting the current image on which it is tapped and than scroll left and right to the other images already present in the chat.

1. This issue does not ask for scrolling right or left.
2. How do you plan to populate the modal with attachments? Can you explain how the logic for that should work?

[HasaanA16]

Hi, sorry for the delayed response.

I did not get it. I pushed my code in a separate branch so that you could test it as there were errors in chat functionality. What do you require exactly ? Did you test my code?

[Jag96]

Putting this issue on HOLD while we sort out https://github.com/Expensify/Expensify.cash/issues/1038#issuecomment-760495360.

[jsamr]

@marcaaron Hi folks! RNRH maintainer here. As I have some free time, I'm crafting a quote for Expansify, migrating RNRH to version 6.0.0-beta.6 with a few optimizations and I stumbled on this thread looking for issues related to my lib. If I remember well, back to alpha.10, an equivalent of passProps was consumable via the useSharedProps hook. As of beta.6, sharedProps is passed to custom renderers directly, and renderersProps are available via useRendererProps. Hope that helps!

[MelvinBot]

@jboniface, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.