quinthar commented 8 months ago

Problem:

Infinite threading means you can get buried deep in a hierarchy and lose track of where you are. We show your ancestors to help with this, but user complaints suggest this isn't enough:

Solution:

Make it look more clear! There was a log discussion on this (Slack) ending with this design:

Specifically:

Add a blue "thread" link to each separator line
Add a gray "replies" message to the final separator
From below - we want the entire message (including the blue thread icon) to be clickable to navigate you to the thread.
From below - clicking on an ancestor message should take you to the thread/room where the message originally appeared, not the thread for the message.

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c49e505032aeb5f6
Upwork Job ID: 1758664864087191552
Last Price Increase: 2024-03-16
Automatic offers:

aimane-chnaif | Reviewer | 0
rayane-djouah | Contributor | 0

melvin-bot[bot] commented 8 months ago

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

melvin-bot[bot] commented 8 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01c49e505032aeb5f6

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

neonbhai commented 8 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Redesign thread ancestr

What is the root cause of that problem?

New Feature

What changes do you think we should make in order to solve the problem?

We will be modifying the thread divider line here in regards to its ancestors:

https://github.com/Expensify/App/blob/6b4246158acd8d202e83668313630fa14b24fedb/src/pages/home/report/ReportActionItemParentAction.tsx#L96

We will:

Wrap Text and Icon and Divider line with a view like this:

<View style={[styles.flexRow, styles.alignItemsCenter, styles.ml5]}>
  <Icon />
  <Text style={[styles.link}>Thread</Text> 
  <View style={[styles.threadDividerLine]} />
</View>

We can put this in a component called ThreadDividerLine
Render a divider line before elements here with text Thread
Take in index as the second argument when mapping ancestors here: https://github.com/Expensify/App/blob/6b4246158acd8d202e83668313630fa14b24fedb/src/pages/home/report/ReportActionItemParentAction.tsx#L77

Render a divider line here according to the logic checking for last element:

{i !== allAncestors.length - 1 ? <ThreadDividerLine text='common.Thread' fill={colors.blue400}/> : <ThreadDividerLine text='common.Replies'/>

Result

Icon will have to be requested from the design team Screenshot 2024-02-17 at 8 28 02 AM

rayane-djouah commented 8 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Redesign thread ancestry

What is the root cause of that problem?

New Feature

What changes do you think we should make in order to solve the problem?

Add a blue "thread" link to each separator line

to do this, we should remove the current thread separator that is displayed below every ancestors here: https://github.com/Expensify/App/blob/91ea9792fe54b795115106965b2682dd1ef36847/src/pages/home/report/ReportActionItemParentAction.tsx#L86-L96

and add a blue "thread" link with a thread divider line above every ancestor ReportActionItem here, we should add this code:

<View style={[styles.flexRow, styles.alignItemsCenter, styles.ml5, index === 0 ? styles.mv2 : [styles.mt3, styles.mb1]]}>
    <PressableWithoutFeedback
        onPress={() =>  Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(ancestor?.report?.parentReportID ?? ''))}
        accessibilityLabel={"Thread"}
        role={CONST.ROLE.BUTTON}
        style={[styles.flexRow, styles.alignItemsCenter, styles.gap1]}
    >
        <Icon
            src={Expensicons.Thread}
            fill={theme.link}
            width={variables.iconSizeExtraSmall}
            height={variables.iconSizeExtraSmall}
        />
        <Text style={[styles.threadDividerText, styles.link]}>Thread</Text> 
    </PressableWithoutFeedback>
    {!ancestor.shouldDisplayNewMarker && <View style={[styles.threadDividerLine]} />} 
</View>

Thread's divider line should hide when the ancestor item is marked as unread, This is so that they will not be conflicting. we will determine if we should display the Thread's divider line with ancestor.shouldDisplayNewMarker, therefore, the ancestor.shouldHideThreadDividerLine will not be needed anymore and we should remove it, here, and here.

From https://github.com/Expensify/App/issues/36752#issuecomment-1967659135 - we want the entire message (including the blue thread icon) to be clickable to navigate you to the thread. From https://github.com/Expensify/App/issues/36752#issuecomment-1979025046 - clicking on an ancestor message should take you to the thread/room where the message originally appeared, not the thread for the message.

we should use PressableWithoutFeedback to make the link clickable.
we should pass Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(ancestor.report.reportID)) to the PressableWithoutFeedback onPress prop to navigate to the thread on click.
we should change the ReportActionItem onPress prop here to:

onPress={() =>  Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(ancestor?.report?.parentReportID ?? ''))}

we should use theme.link and styles.link to make the icon and "Thread" text blue.

For "Thread" and "Replies" text we should use this style:

threadDividerText: {
fontFamily: FontUtils.fontFamily.platform.EXP_NEUE,
fontSize: variables.fontSizeSmall,
textTransform: 'capitalize',
},

we should change the threadDividerLine margins to match the design, instead of marginHorizontal: 20, we should use marginLeft: 8, marginRight: 20,
we should use ancestor index to apply a 8px vertical margin for the first ancestor, and for others we apply 12px top margin and 4px bottom margin.
if we want to display a pointer cursor on the report message, we need to modify the getReportActionItemStyle function here to return styles.cursorPointer in the styles based on a boolean, and we should pass this boolean here as !_.isUndefined(props.onPress) and remove the cursor.cursorAuto style for the message here

Add a gray "replies" message to the final separator

to do this, after displaying all ancestors here, we should add below them this code:

<View style={[styles.flexRow, styles.alignItemsCenter, styles.ml5, styles.mv1]}>
    <Icon
        src={Expensicons.Thread}
        fill={theme.icon}
        width={variables.iconSizeExtraSmall}
        height={variables.iconSizeExtraSmall}
    />
    <Text style={[styles.threadDividerText, styles.textSupporting, styles.ml1]}>Replies</Text> 
    {!shouldHideThreadDividerLine && <View style={[styles.threadDividerLine]} />} 
</View>

Thread's divider line should hide when the first item in the current thread is marked as unread, This is so that they will not be conflicting.
we should use theme.icon and styles.textSupporting to make the icon and "Replies" text gray.

We should use localization function for "Thread" and "Replies" messages.

We can refactor the code into reusable components.

Compare changes

Branch link for testing

Result

https://github.com/Expensify/App/assets/77965000/efd42847-aab4-4148-86b8-cd9cf605e9fc

What alternative solutions did you explore? (Optional)

N/A

dannymcclain commented 8 months ago

Figma file here. cc'ing @Expensify/design on this for visibility.

And here's one more mockup to show more context around how ancestry works:

This look right to you @designteam?

shawnborton commented 8 months ago

That looks right to me, thanks for laying it out like that!

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @dylanexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

mallenexpensify commented 8 months ago

@aimane-chnaif can you please review the proposals above? Added NewFeature to assign a BZ to help move this forward. 👋 @dylanexpensify .

dylanexpensify commented 8 months ago

Nice nice, thanks Matt!

quinthar commented 8 months ago

@aimane-chnaif what's the holdup -- what are the next steps and when will they get done?

aimane-chnaif commented 8 months ago

Ah this is weekly so missed from my radar. 2 proposals to review. will update on Monday

melvin-bot[bot] commented 8 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

mallenexpensify commented 7 months ago

Ah this is weekly so missed from my radar. 2 proposals to review. will update on Monday

New Features default to Weekly, I didn't notice that either. I bumped to Daily.

Victor-Nyagudi commented 7 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Redesign thread ancestry.

What is the root cause of that problem?

No problem here. This is a new feature.

What changes do you think we should make in order to solve the problem?

The new thread and replies markers look very similar to the unread message indicator, so I propose refactoring the UnreadActionIndicator component and re-using some of the logic already present there, that way, if we ever need to make changes to any markers, we can do so in one place.

It also helps with organization and debugging when all related logic is kept together.

Here's how we'll go about this.

Given the current UnreadActionIndicator component will now also act as a thread marker, we'll give it a new, more generic name like MessageDivider.
We'll get the 'comment bubble reply' icon from Figma and add it to the Images folder i.e. root/assets/images.

This icon will be imported in Expensicons.ts and exported accordingly.

The now MessageDivider will need three new props: isThreadDividerLine, isLastThread, and parentReportID.

type UnreadActionIndicatorProps = {
    reportActionID: string;
+    isThreadDividerLine?: boolean;
+    isLastThread?: boolean;
+.   parentReportID?: string | undefined
};

These will be false or an empty string by default.

function UnreadActionIndicator({reportActionID, parentReportID = '', isThreadDividerLine = false, isLastThread = false}: UnreadActionIndicatorProps)

We'll style the current divider line in UnreadActionIndicator according to whether it's for an unread marker or a thread marker.

<View style={isThreadDividerLine ? styles.threadDividerLine : styles.unreadIndicatorLine} />

Next, we'll conditionally render a thread marker or a replies marker based on whether the current thread is the last one.

This marker will be stored in a variable below the current two variables already present.

const threadDividerChildren = (
    <>
        <Icon
            src={Expensicons.CommentBubbleReply}
            fill={isLastThread ? styles.textSupporting.color : styles.link.color}
            width={variables.iconSizeExtraSmall}
            height={variables.iconSizeExtraSmall}
            additionalStyles={styles.mr1}
        />

       <Text style={[styles.unreadIndicatorText, isLastThread ? styles.textSupporting : styles.link]}>{isLastThread ? 'Replies' : 'Thread'}</Text>
    </>
);

const threadDivider = isLastThread ? (
    <View style={[styles.flexRow, styles.alignItemsCenter]}>{threadDividerChildren}</View>
) : (
    <PressableWithoutFeedback
        onPress={() => {
            Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(parentReportID));
        }}
        // Dummy label for now
        accessibilityLabel={'Thread marker link'}
        role={CONST.ROLE.LINK}
        style={[styles.flexRow, styles.alignItemsCenter]}
    >
        {threadDividerChildren}
    </PressableWithoutFeedback>
);

One of the benefits of re-using UnreadActionIndicator component is we won't need to repeat the logic preventing its content from being selected or copied to the clipboard (introduced in this PR to prevent copying content from the mini context menu and established a pattern used in multiple places in the codebase).

This is what the new code in the return statement will look like.

<View
    // This will require a more generic label like 'MessageLineDivider'
    accessibilityLabel={translate('accessibilityHints.newMessageLineIndicator')}
    data-action-id={reportActionID}
    style={[isThreadDividerLine ? styles.threadDividerLineContainer : styles.unreadIndicatorContainer, styles.userSelectNone, !isThreadDividerLine && styles.pointerEventsNone]}
    dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
>
    <View style={isThreadDividerLine ? styles.threadDividerLine : styles.unreadIndicatorLine} />

    {isThreadDividerLine ? threadDivider : <Text style={styles.unreadIndicatorText}>{translate('common.new')}</Text>}
</View>

Over in ReportActionItemParentAction, we'll render the now MessageDivider just before ReportActionItem and pass ancestor.reportAction.reportActionID to UnreadActionIndicator's reportActionID prop.

isLastThread will be false thus the marker rendered will be the one containing the blue icon and the word 'Thread'.

isThreadDividerLine will be true so a thread marker is rendered instead of an unread marker, and we'll pass ancestor.report.parentReportID to parentReportID.

The ReportActionItem will also use the ancestor.report.parentReportID instead of ancestor.report.reportID.

Similarly, we'll conditionally render the now MessageDivider after ReportActionItem, but this time, only if it's the last thread i.e. it's the last ancestor in the allAncestors array.

Since we're already mapping through this array, we can use the index as i in the map function to check if we're on the last item in the array.

We'll also pass the outcome of this check to isLastThread so that when it's true, the replies marker is rendered. We could also explicitly pass true to the prop or store the boolean in a variable somewhere - either works.

Here's what the code from line 78 in ReportActionItemParentAction will now look like.

+ {allAncestors.map((ancestor, i) => (
    <OfflineWithFeedback
        // ... props
    >
+       <UnreadActionIndicator
+           reportActionID={ancestor.reportAction.reportActionID}
+           isThreadDividerLine={true}
+           parentReportID={ancestor.report.parentReportID} 
+       />
             <ReportActionItem
-               onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(ancestor.report.reportID))}
+               onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(ancestor.report.parentReportID))}
                // ... props
             />
+          {!shouldHideThreadDividerLine && i === allAncestors.length - 1 && (
+              <UnreadActionIndicator
+                  reportActionID={ancestor.report.reportID}
+                  isThreadDividerLine={true}
+                  isLastThread={i === allAncestors.length - 1}
+              />
            )}

Lastly, the container housing the thread divider line will have the following styles (added to index.ts in the styles folder).

threadDividerLineContainer: {
    flexDirection: 'row-reverse',
    alignItems: 'center',
    width: '100%',
    paddingHorizontal: 20,
    paddingVertical: 4
},

The thread divider line will have a marginLeft: 8 instead of marginHorizontal: 20 to align with what's in the design file.

threadDividerLine: {
    height: 1,
    backgroundColor: theme.border,
    flexGrow: 1,
-  marginHorizontal: 20,            
+  marginLeft: 8,
},

The words 'Replies' and 'Thread' will be translated using the translate() method once the translations have been verified along with any other copy required.

Short demo after changes

https://github.com/Expensify/App/assets/79470910/af057c19-a2ea-4d92-b2a3-16a145d4e4fa

What alternative solutions did you explore? (Optional)

None as of yet.

Victor-Nyagudi commented 7 months ago

~I apologize in advance for the lack of proper formatting on the code blocks.~

~My computer is at 3%, and I'm nowhere near a charging source. I intend to format them correctly once I've recharged my device.~

~I see the video is uploaded, but it's taking a while to load/play for me. Let me know if you can see it, or if I need to upload it again.~

All good now.

aimane-chnaif commented 7 months ago

Thanks for the proposals everyone. Can you please share demo videos which demonstrate https://github.com/Expensify/App/issues/36752#issuecomment-1952683144?

rayane-djouah commented 7 months ago

Proposal

Updated to match figma design, and added a demo video.

cc @aimane-chnaif

dannymcclain commented 7 months ago

Hey everyone, one thing that I don't think got noted in the issue description is that we want the entire message (including the blue thread icon) to be clickable to navigate you to the thread.

mallenexpensify commented 7 months ago

Added your comment to the OP too @dannymcclain

dylanexpensify commented 7 months ago

cc everyone for above!

Victor-Nyagudi commented 7 months ago

Proposal

Updated

Victor-Nyagudi commented 7 months ago

Hey everyone, one thing that I don't think got noted in the issue description is that we want the entire message (including the blue thread icon) to be clickable to navigate you to the thread.

I'm curious about two things here, @dannymcclain.

The thread ancestor message itself is already quite a large clickable/tapable area, so I wonder if maybe the video in my initial proposal where I clicked on the thread icon to navigate to the ancestor chat gave you the wrong impression.
Should we go through with the icon being clickable, will there be any tooltips on it? The three dot button in the header has a tooltip, so will that pattern also be followed here?

dannymcclain commented 7 months ago

Hopefully this crude mock helps explain it better. I'm just saying that both the pink-overlayed area AND the orange-overlayed area should be clickable, and should navigate you to the thread.

CleanShot 2024-02-28 at 15 43 12@2x

I don't think we need any tooltips on the icon because we have the word "thread" right next to it.

rayane-djouah commented 7 months ago

@dannymcclain The current behavior on production is that when we click on a thread ancestor we navigate to its thread. when we have multiple ancestor, the last one is clickable but it does not navigate because we are already in the corresponding thread. This is IMO a confusing behavior, I think that when we click on a thread ancestor we should navigate to the parent report of the message from where the thread was initiated. What do you think?

Should we navigate to the thread or to it's parent when we click the link?

Current behavior on production:

https://github.com/Expensify/App/assets/77965000/ce5bb30a-3bd7-43ac-966e-4d0dd7a2df08

My proposed behavior:

https://github.com/Expensify/App/assets/77965000/41c4a51e-a31d-4f7c-b7c3-9427881ca2ff

dannymcclain commented 7 months ago

The @Expensify/design team and I were talking about something similar earlier—and basically just trying to figure out the best way for this to work. Would love to get their thoughts on your proposed behavior video. I think I think it makes more sense?

Also, as part of this, shouldn't we be updating the "From..." subheading in the chat header so that it accurately reflects where the thread is coming from?

rayane-djouah commented 7 months ago

To make it more clear:

if we keep current production behavior, when we click on the message or the "Thread" link, we are already on the thread report, and anything will happen on click.

Possible solutions:

Make the thread link navigate to the parent report instead of the thread report.
Keep current behavior and make the last ancestor not clickable.

rayane-djouah commented 7 months ago

Also, as part of this, shouldn't we be updating the "From..." subheading in the chat header so that it accurately reflects where the thread is coming from?

It's done by this PR: https://github.com/Expensify/App/pull/37436 on latest main

dubielzyk-expensify commented 7 months ago

Hmm. This is a tricky one. Cause in some way I'm actually expecting that clickign on the message underneath the thread link will take me to where that message originated from. So basically in the third screen you can't interact with the message, only tap it and then it goes to the second screen. I worry the thread link is too small of a tap target.

I'm happy to lean on @dannymcclain 's intuition here

rayane-djouah commented 7 months ago

So basically in the third screen you can't interact with the message, only tap it and then it goes to the second screen.

Hmm. should we disable all actions on the message and hide context menus? It was not specified in OP.

dannymcclain commented 7 months ago

Does something like this make more sense? Where the current thread message is not tappable. Clicking on one of the "higher up" threads takes you to the room for that thread?

cc @Expensify/design ☝️ Should we take this to Slack to get some clarity?

dannymcclain commented 7 months ago

Hmm. should we disable all actions on the message and hide context menus? It was not specified in OP.

I don't think we need to disable or hide anything for the messages.

Victor-Nyagudi commented 7 months ago

What if we did this.

Infinite threading means you can get buried deep in a hierarchy and lose track of where you are. We show your ancestors to help with this, but user complaints suggest this isn't enough:

The original problem we're trying to tackle is users losing track of where they are in the hierarchy, and the solution is to make things "look" clearer.

Make it look more clear! There was a log discussion on this (Slack) ending with this design:

I'm not sure if there have been user complaints about whether the current navigation to ancestor threads is a pain point or not, so what if we first tackled the visual part, which is a "HIGH" priority, and then address if the thread linking is a problem or not?

There was a log discussion on this (Slack)

It seems a long discussion was already held, so if time is of the essence, I think it would be a good idea to knock things out one at a time.

But then again, if the behavior of clicking/tapping certain ancestor threads and which ones lead to what has always been an issue, I leave it to you to decide if further discussions are necessary or not, and if that should also be handled here.

Victor-Nyagudi commented 7 months ago

Does something like this make more sense? Where the current thread message is not tappable. Clicking on one of the "higher up" threads takes you to the room for that thread?

cc @Expensify/design ☝️ Should we take this to Slack to get some clarity?

@dannymcclain The current thread is tappable/clickable, but it doesn't navigate you somewhere else - only tapping/clicking on the parent navigates you to that thread.

For example, tapping/clicking on "You wanna get pizza?" in the third screen above doesn't navigate you anywhere because you're already on that thread, but clicking on "Same. Let's get some food :pizza:" navigates you to that thread i.e. the second screen.

It sounds like what you want already exists, no?

For what it's worth (even though I know the design team gets the final word), I like what you did here using the blue color to let the user know which ancestor thread is clickable and which doesn't navigate them elsewhere.

Also, as part of this, shouldn't we be updating the "From..." subheading in the chat header so that it accurately reflects where the thread is coming from?

The current behavior is the "From" section shows the parent thread of the current thread you're viewing i.e. where the current thread is from (I don't mean that to sound sarcastic in any way, so please don't take it as such).

For example, the parent of the "You wanna get pizza?" thread in the third image is the "Same. Let's get some food 🍕" thread, and that's what's shown in the header so you can work your way back up, one thread at a time, until you reach the original thread in the workspace.

Doesn't this accurately reflect where the current thread is coming from?

Do you have any examples that could maybe help me better understand what it is you're looking for from the "From" section in the header?

rayane-djouah commented 7 months ago

Does something like this make more sense? Where the current thread message is not tappable. Clicking on one of the "higher up" threads takes you to the room for that thread?

cc @Expensify/design ☝️ Should we take this to Slack to get some clarity?

I like this design! it makes more sense now.

rayane-djouah commented 7 months ago

Proposal

Updated to align with the updated design.

dubielzyk-expensify commented 7 months ago

Should we take this to Slack to get some clarity?

I think so. There's some details to be worked out first.

melvin-bot[bot] commented 7 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 7 months ago

@dylanexpensify, @aimane-chnaif Eep! 4 days overdue now. Issues have feelings too...

dannymcclain commented 7 months ago

Not overdue. Going to get a quick confirmation on expected behavior in Slack today.

dannymcclain commented 7 months ago

Slacked.

dannymcclain commented 7 months ago

Ok coming off of the Slack thread I linked above, I think we are fully read to rock this thing forward. I will update the OP after posting this, but I'm going to post it here too.

Essentially what we've decided is that we want the ancestor message/thread you are currently in to be clickable, and it should take you to the room/thread where the message originally appeared.

So clicking on an ancestor message should take you to where the message was originally posted—not the thread of that message.

Basic mocks:

Clickable area:

Desired interaction:

cc @Expensify/design because this has been a ride haha.

aimane-chnaif commented 7 months ago

Thanks

rayane-djouah commented 7 months ago

Proposal

Updated based on this

Victor-Nyagudi commented 7 months ago

Proposal

Updated.

dylanexpensify commented 7 months ago

Nice, @aimane-chnaif to review!

dylanexpensify commented 7 months ago

@aimane-chnaif bump please

aimane-chnaif commented 7 months ago

I've been checking through all design discussions, along with proposals. Expecting update on Monday

melvin-bot[bot] commented 7 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 7 months ago

@dylanexpensify, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

Expensify / App

[HOLD for payment 2024-05-15] HIGH: [Polish] [$500] Redesign thread ancestry #36752

Problem:

Solution:

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Result

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Proposal

Proposal

Proposal

Proposal

Proposal