-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[Hold for payment 2023-03-17] [$2000] [Feature] We should be able to cycle between opened media in a chat - reported by @sig5 #7862
Comments
Triggered auto assignment to @trjExpensify ( |
Triggered auto assignment to @megankelso ( |
Oh, this is cool! What are we thinking in terms of the UI for the controls for this? We should probably loop in design before moving this on to further, so I'm adding the In the meantime, here are some quick thoughts:
CC: @shawnborton |
Yeah I'm mostly curious what we consider to be the total batch of attachments that you would cycle between. Is it only the attachments on a given message/thread, or could you cycle through all attachments across all messages in a chat? Taking a look at Slack, it seems like on desktop you can only cycle through the group of attachments found throughout an entire message + subsequent thread messages. On mobile, it seems like you can cycle through everything found in a given chat/room. @trjExpensify do you prefer one or the other? Maybe we can take this to the room for a quick discussion too. |
Yeah, good question. I personally prefer being able to cycle through all attachments across all messages in a chat. The limit on message + threaded messages on Slack desktop is always a bit of a pain, IMO. A neat thing WhatsApp does if you exit out of the attachment is navigate you back in the chat history to where it was originally posted for the added context in and around when it was sent. Posted on the original thread here. |
Followed up in the slack thread, sorry I missed the replies! 😅 |
Pulling a couple of things out of the Slack thread..
We don't have threads yet, and similarly one message is one attachment, so cycling through all attachments across all messages in a chat seems to be where we're at here?
I think we can probably descope this for now, though I agree it would be nice for speed scrolling. Especially if you know you're trying to get back to a point in time looking for something.
As I mentioned in the thread, I don’t have a strong opinion on looping back to the most recent/oldest when you reach the end/beginning. I think we’d need to make that clear if we allowed for it though, which presumably means including some indication of date/time as well. Otherwise you'd have the perception of infinitely scrolling back, unaware that you've looped back to the start. |
Agree with your first two points. I think we can descope wrapping too. |
Nice, works for me! 👍 |
Okay cool, so what are the next steps here? We're ready to mock it up at this point, right? |
That makes sense to me - looks like this is already assigned to @megankelso. @trjExpensify can you update the original comment to reflect what the desired solution/expected result should be, and then Megan can make some mocks? I think we'll just need some left/right arrows to exist in the attachment viewer modal. |
Okay cool, OP has been updated! The only thing I'm unsure of for how we're going to handle at this point is this one:
|
I like the idea of using hover for web. On mobile, we could just do it where a single tap on the image hides/shows the arrows? |
Works for me! 👍 |
I think it's pretty standard that the arrows would always be on the left & right edges of the screen, so I prefer those mockups in comparison to the mockups where the arrows are below the image. My concern with using white arrows is that if the attachment was white, you wouldn't see the arrow. Could we see a version where the arrows just use our standard button styling? Perhaps even borrowing some of the round styles you used for the video call mocks you made. |
PR review is ongoing Melvin, chill. |
@JediWattson, @chiragsalian, @trjExpensify, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@JediWattson, @chiragsalian, @trjExpensify, @Santhosh-Sellavel Huh... This is 4 days overdue. Who can take care of this? |
@JediWattson, @chiragsalian, @trjExpensify, @Santhosh-Sellavel 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it! |
Close to completion! |
This appears to be live on production?!??!? |
@JediWattson, @chiragsalian, @trjExpensify, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Oh hm, the title didn't update automatically. Done that! @Santhosh-Sellavel @chiragsalian @luacmartins what are the next steps on the follow-up items? Seems like the most pressing item is figuring out how to "go back" further versus only as far as reportActions we have loaded, right? I think we'd go ahead and create a new issue for that. |
Create an issue for each, discuss within each if they are worth it at this time, and then mark them all as external.
Personally yeah I would like to see this one implemented. It will involve some backend changes. |
What are those? I seem to recall the debounce improvement and going back further being the only ones worth continuing.
Okay cool, so shall we have an |
@JediWattson, @chiragsalian, @trjExpensify, @Santhosh-Sellavel Huh... This is 4 days overdue. Who can take care of this? |
@JediWattson, @chiragsalian, @trjExpensify, @Santhosh-Sellavel 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
Alrighty, I've sent offers for the increased values: @JediWattson $4k for the fix |
Settled up with @JediWattson ✅ , over to you @Santhosh-Sellavel! |
Settled up with @Santhosh-Sellavel ✅, we're done here. Closing! |
I think I have not been paid the bounty for the 🐛 |
📣 @sig5! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Format:
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
Hey, @sig5. This was a feature request and not a bug, so we don't pay a bug report bounty for those: https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#proposing-a-job-that-expensify-hasnt-posted |
https://github.com/Expensify/App/blob/b1a08ca2e41e5f43f4865c6518a2813a21b4b514/CONTRIBUTING.md It wasn't the case when the issue had started. Seems like there was a change in rules in the meanwhile. But I think old rules should be honoured because it was reported then. |
Bump### |
Need to get done with this, So BUMP @trjExpensify |
Hey @sig5, easy on the caps lock, I've been out of office until today. 🙂 I checked the feature request freeze and you're right it did come into play after the issue creation. I've sent you a contract for the bug report on Upwork. |
Ayy, thanks man. Didn't mean to be rude if felt that way. |
All good, that's settled now. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Problem:
It will be a nice UX touch for the user, adding convenience while finding an old media file.
Solution:
Add an option to cycle between media when opened via swipe/arrow keys/buttons.
Action Performed:
Expected Result:
Actual Result:
We don't support cycling through attachments in a chat currently.
Workaround:
Close the attachment modal, then scroll to the next attachment and open it.
Platform:
Where is this issue occurring?
Version Number: 1.1.39-1
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Upwork URL: https://www.upwork.com/jobs/~011ca7e66b62951030
Issue reported by: @sig5
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644597095180509
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: