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

[quinthar]

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]

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]

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

[melvin-bot]

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

[neonbhai]

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:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 96 in 6b42461

{!ancestor.shouldHideThreadDividerLine && <View style={[styles.threadDividerLine]} />}

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:

  App/src/pages/home/report/ReportActionItemParentAction.tsx

  Line 77 in 6b42461

  {allAncestors.map((ancestor) => (

• 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

[rayane-djouah]

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?

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

App/src/pages/home/report/ReportActionItemParentAction.tsx

Lines 86 to 96 in 91ea979

	<ReportActionItem
	// @ts-expect-error TODO: Remove this once ReportActionItem (https://github.com/Expensify/App/issues/31982) is migrated to TypeScript.
	onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(ancestor.report.reportID))}
	report={ancestor.report}
	action={ancestor.reportAction}
	displayAsGroup={false}
	isMostRecentIOUReportAction={false}
	shouldDisplayNewMarker={ancestor.shouldDisplayNewMarker}
	index={index}
	/>
	{!ancestor.shouldHideThreadDividerLine && <View style={[styles.threadDividerLine]} />}

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 #36752 (comment) - we want the entire message (including the blue thread icon) to be clickable to navigate you to the thread.
From #36752 (comment) - 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

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

Threads.Redesign.Recording.2024-03-05.174204.mp4

What alternative solutions did you explore? (Optional)

N/A

[dannymcclain]

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]

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

[melvin-bot]

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]

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

[dylanexpensify]

Nice nice, thanks Matt!

[quinthar]

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

[aimane-chnaif]

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

[melvin-bot]

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

[mallenexpensify]

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.

[dylanexpensify]

Similar

[dannymcclain]

Looks like 39982 has been deployed.

[rayane-djouah]

Off hold, The PR is merged

[dylanexpensify]

NOICE!

[dylanexpensify]

PR not yet on staging, waiting

[dylanexpensify]

Payment summary:

• Contributor: @rayane-djouah $750
• Contributor+: @allroundexperts $750

Please apply/request!

[melvin-bot]

@dannymcclain, @chiragsalian, @dylanexpensify, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[rayane-djouah]

PR deployed on May 9

[dylanexpensify]

updated to reflect

[dylanexpensify]

payment tomorrow!

[rayane-djouah]

Please correct the payment summary, the payment amount is 500$ not 750$

[melvin-bot]

@dannymcclain, @chiragsalian, @dylanexpensify, @rayane-djouah Eep! 4 days overdue now. Issues have feelings too...

[dylanexpensify]

@rayane-djouah we decided $750 given there was a follow on issue created and done that wasn't necessarily needed to solve the bug, but a polish. Does that sound right? cc @chiragsalian

[rayane-djouah]

@dylanexpensify I trust your judgment on what you find fair. Thanks!

[dannymcclain]

Not overdue, right @dylanexpensify?

[dylanexpensify]

Nope! Just finishing payments!

[dylanexpensify]

Done!

[JmillsExpensify]

$750 approved for @allroundexperts