mvtglobally commented 2 years ago

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:

Navigate to any chat
Send multiple attachment
Try to navigate through older attachments/media

Expected Result:

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.
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.
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.

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?

Web
iOS
Android
Desktop App
Mobile Web

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

MelvinBot commented 2 years ago

Triggered auto assignment to @trjExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

MelvinBot commented 2 years ago

Triggered auto assignment to @megankelso (Design), see these Stack Overflow questions for more details.

trjExpensify commented 2 years ago

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 design label for now and dropping the priority to weekly. After which, we can update the OP.

In the meantime, here are some quick thoughts:

When the member is on the most recent attachment in the chat, there's only a left arrow icon (centered and left-aligned?) in the preview screen.
If the member is previewing an attachment "further back" in the history, there will also be a right arrow icon (centered and right-aligned?) to navigate "forward" to the next attachment preview
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
Should we give members the ability to dismiss/hide the navigation arrows in some capacity? On web/desktop I can see maybe only revealing the arrow(s) on hover? On smaller screens like mobile/mWeb they might get in the way, if we don't offer the ability to tap to dismiss/hide them in some capacity?

CC: @shawnborton

shawnborton commented 2 years ago

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.

trjExpensify commented 2 years ago

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.

trjExpensify commented 2 years ago

Followed up in the slack thread, sorry I missed the replies! 😅

trjExpensify commented 2 years ago

Pulling a couple of things out of the Slack thread..

Is it only the attachments on a given message/thread, or could you cycle through all attachments across all messages in a chat?

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?

Should we include a horizontal scrollview like WhatsApp?

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.

Should the carousel of attachments "wrap". i.e: if I have the last attachment open, and I press ArrowLeft am I taken to the first attachment or does nothing happen? Is the > in the carousel disabled, or does it take me back to the beginning?

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.

shawnborton commented 2 years ago

Agree with your first two points. I think we can descope wrapping too.

trjExpensify commented 2 years ago

Nice, works for me! 👍

trjExpensify commented 2 years ago

Okay cool, so what are the next steps here? We're ready to mock it up at this point, right?

shawnborton commented 2 years ago

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.

trjExpensify commented 2 years ago

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:

Should we give members the ability to dismiss/hide the navigation arrows in some capacity? On web/desktop I can see maybe only revealing the arrow(s) on hover? On smaller screens like mobile/mWeb they might get in the way, if we don't offer the ability to tap to dismiss/hide them in some capacity?

shawnborton commented 2 years ago

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?

trjExpensify commented 2 years ago

On mobile, we could just do it where a single tap on the image hides/shows the arrows?

Works for me! 👍

megankelso commented 2 years ago

some ideas for this

the hover/tap made me think y'all were suggesting the arrows as an overlay, would probably need to style differently but on first pass I prefer them not appearing over the image. curious for thoughts

shawnborton commented 2 years ago

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.

shawnborton commented 2 years ago

Also a small nitpick: can you update the screenshots to use the current attachment modal styling? It has a subheader for the filename as well as a bottom border:

megankelso commented 2 years ago

I'm not sure what our standard button styling is but here are the ones I know of. I think the icon alone looks best but even the grey disappears on some images (big time on this one)

megankelso commented 2 years ago

also, do they the arrows in the circles feel a bit big to you? there's at 20 px but I wonder if we could bump down to 16 to allow for a smaller circle.

shawnborton commented 2 years ago

You can find all of our standard button styles in our component library, here is for reference though:

So you have the sizes of:

Small: 28px
Medium: 40px
Large: 52px

I think we'd want these to be at least medium size so they are reasonably tappable on mobile devices. So that being said, 40px button with a 20px icon might feel good.

I think having a gray background is a good idea because this way we guarantee that the arrow will be visible on this gray background, even if the gray gets lost on the image. Also, this way we stay consistent and we don't introduce a new button style.

megankelso commented 2 years ago

here's the 40/20 button size. also it appears vertical photos may not appear with a border on mobile, so I updated that.

I personally think these looks pretty chunky and prefer the arrows alone below the image. For mobile, I assume most people will swipe to see other attachments so I think including these are mostly to show you can scroll.

I can understand how the 40/20 buttons are more standard though. If we do the tap to hide/show, perhaps they won't feel so obtrusive.

shawnborton commented 2 years ago

Personally I think that looks great. Again, with arrows only, they can easily get lost depending on the image that is behind them so we very easily run into a usability issue. This also feels consistent with other button styles.

megankelso commented 2 years ago

I was suggesting the arrows go below the image not on top.

Anyway, understand this solution feels more consistent. Thinking web would look something like this then

shawnborton commented 2 years ago

For web, I was thinking the arrows would be closer to the edge of the screen. Especially since the attachment could be wider. Here is what Slack does for reference:

Thoughts?

megankelso commented 2 years ago

yeah we could move them over

shawnborton commented 2 years ago

Cool, I think that makes more sense. Thanks!

megankelso commented 2 years ago

ok for easy reference, mockups here! I think this is all we need.

trjExpensify commented 2 years ago

Purrrrfect, thanks! 😸

Updated the OP to include the mocks and passing this over to engineering triage before going external. 👍

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @alex-mechler (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

alex-mechler commented 2 years ago

I didnt realize how much I needed this before I saw this issue 😅

Looks good to go external!

melvin-bot[bot] commented 2 years ago

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @chiragsalian (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

trjExpensify commented 2 years ago

Job on Upwork here: https://www.upwork.com/jobs/~011ca7e66b62951030

Santhosh-Sellavel commented 2 years ago

@trjExpensify Let's Double this one!

cc: @chiragsalian

trjExpensify commented 2 years ago

Yep, back from OoO today. Doubled the job on Upwork.

chiragsalian commented 2 years ago

Waiting on proposals.

Santhosh-Sellavel commented 2 years ago

I think it's time to double this! cc: @trjExpensify @chiragsalian

trjExpensify commented 2 years ago

Doubled on Upwork here.

Santhosh-Sellavel commented 2 years ago

Let's double this one up @trjExpensify.

cc: @chiragsalian

trjExpensify commented 2 years ago

Doubled!.

JediWattson commented 2 years ago

Proposal

Data flow

Use the navigator to retrieve the reportId from within the ImageRenderer. Navigation.getActiveRoute().replace('/r/', "")

This allows the modal to use Onyx and retrieve the reportActions for the proper id. From here a function called show is called when an image is pressed inside a chat, which can be used to create an array of attachment urls from the reportActions data. The array's findIndex is called to get the matching index of the current url and a page is set up to track the index. An event on press for the arrow will check if the page is within bounds and iterate along with changing the sourceURL on the state.

UI

The arrows are set up using a View component with flexbox and absolute position, they sit next to the attachment view with a boolean that disables them is the reportId doesn't exist. I think there might a couple places I need to add these arrows so I might just create a function to reduce code.

The Story so Far

I know the arrows don't match the previous designs, but I wanted to get the positioning right before exploring what's available with the icons and any use cases I should cover with disabling them.

Santhosh-Sellavel commented 2 years ago

@JediWattson Seems you are new here, welcome! Thanks for your interest to solve this, the results look promising!

Would you please elaborate more on technical code changes? so that I can review the proposal.

Look at this past jobs as a reference to write proposals!

And If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

JediWattson commented 2 years ago

Proposal v2

I actually made a few changes to the previous, but I'll get into more details with code examples this time!

I'll start with the modal showing, which seems to be this callback function that sets the state in the modal. I added a piece of data to obtain the report id here.

show: () => {                        
    const reportId = Navigation.getActiveRoute().replace('/r/', "")
    this.setState({ reportId, isModalOpen: true});
},

the report id, along with a callback function from the modal are sent to a component. The current url is sent so the current index can be found in the creation of the attachment array.

{this.state.reportId && ( 
    <AttachmentNav 
        reportId={this.state.reportId}                                    
        onArrowPress={this.onArrowPress}
        sourceURL={this.state.sourceURL}
    />
)}

I moved most of what I described before into a component, which is wrapped in withOnyx to provide the data for attachments.

export default withOnyx({
    reportActions: {
        key: ({reportId}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportId}`,
        canEvict: true
    },
})(AttachmentsNav)

I start the creation of the attachment array by converting the actions to an array using Object.values, this value also is used as a dependency in useEffect by determining if the function should be called based on the length of this array.

const actionsArr = Object.values(reportActions)

I use the reduce function to essentially filter and map the actions into only the urls for attachments

        const nextAttachments = actionsArr.reduce((arr, rep, index) => {
            const matches = CONST.REGEX.ATTACHMENT_SRC.exec(rep.originalMessage?.html)
            if(matches){
                if(matches[1].includes(sourceURL))
                    setPage(arr.length)        
                const url = matches[1].replace(
                    CONFIG.EXPENSIFY.EXPENSIFY_URL,
                    CONFIG.EXPENSIFY.URL_API_ROOT,
                )
                arr.push(url)
            }                                    
            return arr
        }, [])
        setAttachments(nextAttachments)

I guess a few things to note here as well is, I'm using a function component for AttachmentsNav and the useState hook to hold both the attachments along with a page or index that I initialize with the matching sourceURL. Finally I tried to use the replace function along with these url constants provided to prevent CORS I believe.

Next I'll show the pagination here, and I'll start with the jsx because it's where the event starts.

<View style={{ width: "90%", position: "absolute",  justifyContent: "space-between", alignItems: "center", flexDirection: "row" }}>
    <Pressable onPress={() => handlePress(true)} style={{ cursor: "pointer" }}>
        <Icon src={BackArrow} height={42} width={42} />
    </Pressable>        
    <Pressable onPress={() => handlePress()} style={{ cursor: "pointer" }}>
        <Icon src={ArrowRight} height={42} width={42} />
    </Pressable>
</View>

I'll say right off the bat that I should use constants for styles here, in fact I'll try to get that in later. But what you see in the Pressable component is an event onPress that calls a function handlePress. The idea is to keep the code clean and send true if it's a back press and I'll show the function here to get a better understanding.

function handlePress(isBack){
    if(isBack ? page-1 < 0 : page+1 === attachments.length) return        
    const nextIndex = isBack ? page-1 : page+1        
    onArrowPress(attachments[nextIndex])
    setPage(nextIndex)        
}

Here it starts with a guard to check the index is within bounds, then appropriately changes the index and set it along with the new sourceURL, which is sent to the parent component.

onArrowPress(sourceURL){
    this.setState({sourceURL})
}

now the modal will reload the with the new url and it'll look like the use is navigating through the chat's attachments.

Thanks for your consideration!

Santhosh-Sellavel commented 2 years ago

@JediWattson I appreciate your effort, Sorry I am not able to follow you completely. Can you share the full proposal or elaborate a bit more clearly.

Example:

Let's make a new Carousel Component to cycle to the images.

// Full Code for the component
// Overview of component code snippets

Use it in the Existing component Image, making the following changes.
```
// Changes snippet and elaborate why if needed.
```
.....

JediWattson commented 2 years ago

Proposal

Sure thing, I think I've been looking at bug proposals when I should have picked out a feature from the link you posted. I certainly don't mind trying to get this right.

Make a Carousel component to cycle through images. (I can change the name to AttachmentCarousel)
```
/// imports up here
```

function AttachmentsNav({ reportActions, onArrowPress, sourceURL }) { const actionsArr = Object.values(reportActions) const [page, setPage] = useState(-1) const [attachments, setAttachments] = useState([]) useEffect(function(){ const nextAttachments = actionsArr.reduce((arr, rep, index) => { const matches = CONST.REGEX.ATTACHMENT_SRC.exec(rep.originalMessage?.html) if(matches){ if(matches[1].includes(sourceURL)) setPage(arr.length)
const url = matches[1].replace( CONFIG.EXPENSIFY.EXPENSIFY_URL, CONFIG.EXPENSIFY.URL_API_ROOT, ) arr.push(url) }
return arr }, []) setAttachments(nextAttachments)

}, [actionsArr.length])

function handlePress(isBack){
    if(isBack ? page-1 < 0 : page+1 === attachments.length) return        
    const nextIndex = isBack ? page-1 : page+1        
    onArrowPress(attachments[nextIndex])
    setPage(nextIndex)        
}

return (
    <View style={{ width: "90%", position: "absolute",  justifyContent: "space-between", alignItems: "center", flexDirection: "row" }}>
        <Pressable onPress={() => handlePress(true)} style={{ cursor: "pointer" }}>
            <Icon src={BackArrow} height={42} width={42} />
        </Pressable>        
        <Pressable onPress={() => handlePress()} style={{ cursor: "pointer" }}>
            <Icon src={ArrowRight} height={42} width={42} />
        </Pressable>
    </View>                            
)

}

export default withOnyx({ reportActions: { key: ({reportId}) => ${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportId}, canEvict: true }, })(AttachmentsNav)


2. Use the existing component AttachmentModal, making the following changes

in the show function here:
https://github.com/Expensify/App/blob/75d9014c75befb7655e78653c65f4d2e0c0f0cdf/src/components/AttachmentModal.js#L227

I added reportId to the state update which I get using the Navigation route
```js
show: () => {                        
    const reportId = Navigation.getActiveRoute().replace('/r/', "")
    this.setState({ reportId, isModalOpen: true});
}

the carousel component will sit next to the AttachmentView component here https://github.com/Expensify/App/blob/75d9014c75befb7655e78653c65f4d2e0c0f0cdf/src/components/AttachmentModal.js#L183

this will render if reportId exists

{this.state.reportId && ( 
    <AttachmentNav 
        reportId={this.state.reportId}                                    
        onArrowPress={this.onArrowPress}
        sourceURL={this.state.sourceURL}
    />
)}

lastly, there's a callback function added to AttachmentModal called onArrowPress, this will set the new url from the carousel component.

onArrowPress(sourceURL){
    this.setState({sourceURL})
}

chiragsalian commented 2 years ago

Still discussing proposals. Not overdue. Also demoting to weekly.

Santhosh-Sellavel commented 2 years ago

@JediWattson can you share ATTACHMENT_SRC regex?

JediWattson commented 2 years ago

That would look like:

{
    ...,
    ATTACHMENT_SRC: /data-expensify-source="([^"]+)"/
}

I found it in this forum and modified it to look at data-expensify-source since that seemed unique to attachments that I found in this boolean https://github.com/Expensify/App/blob/75d9014c75befb7655e78653c65f4d2e0c0f0cdf/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js#L30

Santhosh-Sellavel commented 2 years ago

I'm not sure what I am missing pressing next or back not working, can you share a working video?

@JediWattson

Expensify / App

[Hold for payment 2023-03-17] [$2000] [Feature] We should be able to cycle between opened media in a chat - reported by @sig5 #7862

Problem:

Solution:

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

Proposal

Data flow

UI

The Story so Far

Proposal v2

Proposal