Attachment cycling with onyx #1210
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
| @@ -209,7 +210,7 @@ class ReportActionCompose extends React.Component { | |||
|
|
|||
| </> | |||
| )} | |||
| </AttachmentModal> | |||
| </ComposeAttachmentModal> | |||
There was a problem hiding this comment.
I'm confused about why we are making this more specific. Could we just leave this as an AttachmentModal?
There was a problem hiding this comment.
Here:
#1149 (review)
tgolen suggests having separate components to keep the code clean, therefore that's why I've changed the component to its own separate ComposeAttachmentModal.
There was a problem hiding this comment.
@tgolen That seems ok to me, but the consequence is that we have shared logic being duplicated.
There was a problem hiding this comment.
At this point I would say the only thing they share is that they instantiate a modal, their purposes are quite different now and will likely get more different in the future if you decide to add upload progress .etc to the compose modal.
I believe writing a singular modal to fulfil both use cases would be more confusing.
| sourceURL, isAttachment, isModalOpen, file, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not really following why we are storing the entire state of the AttachmentModal in Onyx. We should only need to store the information we need. Which I think is just something like the activeAttachmentSourceUrl.
There was a problem hiding this comment.
also file can be an instance of File which I think might not work well with storage.
There was a problem hiding this comment.
Doesn't this only occur when the file is uploaded, therefore would only be shown in the ComposeAttachmentModal component?
There was a problem hiding this comment.
Also we're storing the state like this because the AttachmentModal's isModalOpen property needs to be controlled from a centralised position.
There was a problem hiding this comment.
Ok, so we need the sourceURL, isAttachment, and isModalOpen? Do we need file?
There was a problem hiding this comment.
I have no clue what the initial purpose of what the file attribute was in the codebase, I assume it was to show the file name for PDFs or something. But I simply kept it because I didn't think it was my place to be deleting functionality
|
also adding @tgolen |
- Changed filter to use underscore - Fixed modal listener logic where listener would stop workin after first trigger.
changes getNextAttachment argument from toRight to direction. Removed object destructuring for readability.
- KeyboardShortcut usages instead of document. - AttachmentModal only now receives attachments - Removed reportID.file.name from JSDoc definition
|
Closing PR. Marc and i discussed about this issue for a while and we feel like we should close this PR for now. We'll be splitting the original GH issue into multiple smaller issues to discuss and improve renderHTML and cycling attachments and once we've settled with a technical plan we'll be posting those in upwork. Thank you for your time and work. Closing PR now. |
Implemented functionality to achieve the following statement:
"The attachments present in the chat should cycle when the user is viewing the modal and presses the left/right arrows on their keyboard"
Details
Refactored code and moved Attachment modal so only one is created for ReportActionsView and one for the ReportActionCompose view. AttachmentModal now consumed visibleModal object from Onyx to know what to show and wether to be visible.
Also wrote getNextAttachment function which retrieves the next attachment using the current action and a boolean value indicating whether to move to the right or left of the current action.
Fixed Issues
Implements #1038
Tests
Upload multiple attachments (Images and PDFs)
Once they have been uploaded, use the left and right arrow keys to move between all of them.
Go to the last attachment and confirm that moving to the right of the last attachment takes you to the first attachment in a cyclical manner.
Test that uploading attachments still work (Images, PDFs .etc)
Tested On
Screenshots
Web
Mobile Web
no changes
Desktop
no changes
iOS
no changes
Android
no changes