Skip to content
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 2024-10-16] [HOLD for payment 2024-10-14] [HOLD for payment 2024-10-11] [$250] Task - Long Task title is overlapped by task arrow #49086

Closed
1 of 6 tasks
IuliiaHerets opened this issue Sep 12, 2024 · 69 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Design External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 12, 2024

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


Version Number: 9.0.33-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4955214&group_by=cases:section_id&group_order=asc&group_id=291935
Email or phone of affected tester (no customers): gocemate+a2096@gmail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to Group chat> +> Assign Task
  2. In title field enter long text> Finish the flow
  3. Take a look at Task title in Group chat

Expected Result:

Task title should not be overlapped by arrow

Actual Result:

Task title is overlapped by task arrow

Workaround:

Unknown

Platforms:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6600811_1726147239222.Recording__3921.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021834296909071158877
  • Upwork Job ID: 1834296909071158877
  • Last Price Increase: 2024-09-19
  • Automatic offers:
    • rayane-djouah | Reviewer | 104113003
Issue OwnerCurrent Issue Owner: @Christinadobrzyn
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 12, 2024
Copy link

melvin-bot bot commented Sep 12, 2024

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@IuliiaHerets
Copy link
Author

@Christinadobrzyn FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@Krishna2323
Copy link
Contributor

Krishna2323 commented Sep 12, 2024

Proposal


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

Task - Long Task title is overlapped by task arrow

What is the root cause of that problem?

styles.flex1 is not added to the container of RenderHTML.

<View style={[styles.alignSelfCenter]}>
<RenderHTML html={isTaskCompleted ? `<completed-task>${htmlForTaskPreview}</completed-task>` : htmlForTaskPreview} />
</View>

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


Add styles.flex1 to the container of RenderHTML.

OPTIONAL: Also add styles.flexRow, styles.alignItemsCenter, styles.breakWord, styles.preWrap & styles.renderHTML if needed.

OPTIONAL 2: We can move the ArrowRight in this container and wrap it with a view and also apply styles.alignSelfCenter to the view to centre the icon.

What alternative solutions did you explore? (Optional)

Result

@ChavdaSachin
Copy link
Contributor

ChavdaSachin commented Sep 12, 2024

Edited by proposal-police: This proposal was edited at 2024-09-12 14:13:45 UTC.

Proposal

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

Task - Long Task title is overlapped by task arrow

What is the root cause of that problem?

  • styles.flex1 is missing from RenderHTML container. And icon is not justifiedCenter.
    <View style={[styles.flex1, styles.flexRow, styles.alignItemsStart, styles.mt1]}>
    <View style={[styles.taskCheckboxWrapper, styles.alignSelfCenter]}>
    <Checkbox
    style={[styles.mr2]}
    isChecked={isTaskCompleted}
    disabled={!Task.canModifyTask(taskReport, currentUserPersonalDetails.accountID)}
    onPress={Session.checkIfActionIsAllowed(() => {
    if (isTaskCompleted) {
    Task.reopenTask(taskReport);
    } else {
    Task.completeTask(taskReport);
    }
    })}
    accessibilityLabel={translate('task.task')}
    />
    </View>
    {taskAssigneeAccountID > 0 && (
    <Avatar
    containerStyles={[styles.mr2, styles.alignSelfCenter, isTaskCompleted ? styles.opacitySemiTransparent : undefined]}
    source={avatar}
    size={CONST.AVATAR_SIZE.SMALL}
    avatarID={taskAssigneeAccountID}
    type={CONST.ICON_TYPE_AVATAR}
    />
    )}
    <View style={[styles.alignSelfCenter]}>
    <RenderHTML html={isTaskCompleted ? `<completed-task>${htmlForTaskPreview}</completed-task>` : htmlForTaskPreview} />
    </View>
    </View>
    <Icon
    src={Expensicons.ArrowRight}
    fill={StyleUtils.getIconFillColor(getButtonState(isHovered))}
    />

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

  • following code perfectly aligns with the expectations
    <View style={[styles.flex1, styles.flexRow, styles.mt1]}>
        <View style={[styles.taskCheckboxWrapper,taskAssigneeAccountID > 0 && styles.mt1]}>
            <Checkbox
                style={[styles.mr2, styles.alignSelfEnd]}
                isChecked={isTaskCompleted}
                disabled={!Task.canModifyTask(taskReport, currentUserPersonalDetails.accountID) || !Task.canActionTask(taskReport, currentUserPersonalDetails.accountID)}
                onPress={Session.checkIfActionIsAllowed(() => {
                    if (isTaskCompleted) {
                        Task.reopenTask(taskReport);
                    } else {
                        Task.completeTask(taskReport);
                    }
                })}
                accessibilityLabel={translate('task.task')}
            />
        </View>
        {taskAssigneeAccountID > 0 && (
            <Avatar
                containerStyles={[styles.mr2,styles.alignSelfStart, isTaskCompleted ? styles.opacitySemiTransparent : undefined]}
                source={avatar}
                size={CONST.AVATAR_SIZE.SMALL}
                avatarID={taskAssigneeAccountID}
                type={CONST.ICON_TYPE_AVATAR}
            />
        )}
        <View style={[taskAssigneeAccountID > 0 ? styles.alignSelfStart:styles.alignSelfCenter, styles.flex1 ]}>
            <RenderHTML html={isTaskCompleted ? `<completed-task>${htmlForTaskPreview}</completed-task>` : htmlForTaskPreview} />
        </View>
        <Icon
            additionalStyles={[styles.alignSelfStart,taskAssigneeAccountID > 0 && styles.mt1]}
            src={Expensicons.ArrowRight}
            fill={StyleUtils.getIconFillColor(getButtonState(isHovered))}
        />
    </View>

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@Krishna2323
Copy link
Contributor

Proposal Updated

  • Added optional 2

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Sep 12, 2024
@melvin-bot melvin-bot bot changed the title Task - Long Task title is overlapped by task arrow [$250] Task - Long Task title is overlapped by task arrow Sep 12, 2024
Copy link

melvin-bot bot commented Sep 12, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 12, 2024
Copy link

melvin-bot bot commented Sep 12, 2024

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

Copy link

melvin-bot bot commented Sep 12, 2024

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

@Christinadobrzyn
Copy link
Contributor

I can reproduce this. I think it can be external and I also think we need design to review this to see what would be best for the layout of the arrow.

@dannymcclain
Copy link
Contributor

dannymcclain commented Sep 12, 2024

I'm pretty sure this is a regression. Not sure from where but this definitely used to not happen. We just need to wrap the text to multiple lines like we used to.

Maybe from the recent Tasks updates we did to display the user's avatar instead of an @mention?

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Sep 12, 2024

Ah thanks @dannymcclain - let me dig into any PRs that might relate...

There are a lot of task GHS - asking QA if they can help me narrow this down - https://expensify.slack.com/archives/C9YU7BX5M/p1726169064112539

@Christinadobrzyn
Copy link
Contributor

Hey @thienlnam this is a regression that was found when executing this GH. Do you happen to know which PR this might relate to?

@thienlnam
Copy link
Contributor

thienlnam commented Sep 12, 2024

I'm not sure, maybe this one? #48552

EDIT: Nvm, that one isn't even deployed yet. Yeah I'm not sure, probably has been like this for a while

@Christinadobrzyn
Copy link
Contributor

@rayane-djouah would you be able to help us look into what this might be a regression of? Or, I think if we can't find the original PR we should consider just fixing this as a new job. @rayane-djouah let me know your thoughts

@bernhardoj
Copy link
Contributor

Reverting #48552 locally does fix the issue for me.

Inspecting the element on prod shows that the element structure matches the changes from the PR.
image

A View (div) with an align self: center and a children of the RenderHTML.

@Gajendra-Gupta
Copy link

Proposal

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

Task - Long Task title is overlapped by task arrow

What is the root cause of that problem?

App/src/components/ReportActionItem/TaskPreview.tsx

Lines 122 to 124 in 65a21fc

<View style={[styles.alignSelfCenter]}> 
         <RenderHTML html={isTaskCompleted ? `<completed-task>${htmlForTaskPreview}</completed-task>` : htmlForTaskPreview} /> 
     </View>

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

We need to replace this code

<View style={[styles.alignSelfCenter]}> 
         <RenderHTML html={isTaskCompleted ? `<completed-task>${htmlForTaskPreview}</completed-task>` : htmlForTaskPreview} /> 
     </View>

by

<RenderHTML html={isTaskCompleted ? `<completed-task>${htmlForTaskPreview}</completed-task>` : htmlForTaskPreview} />

Also If you want arrow in center then need to add style styles.alignItemsCenter on this

<PressableWithoutFeedback

so code will be

<PressableWithoutFeedback
                onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(taskReportID))}
                onPressIn={() => DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
                onPressOut={() => ControlSelection.unblock()}
                onLongPress={(event) => showContextMenuForReport(event, contextMenuAnchor, chatReportID, action, checkIfContextMenuActive)}
                shouldUseHapticsOnLongPress
                style={[styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter]}
                role={CONST.ROLE.BUTTON}
                accessibilityLabel={translate('task.task')}
            >

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Sep 16, 2024
@rayane-djouah
Copy link
Contributor

Will review today!

@melvin-bot melvin-bot bot removed the Overdue label Sep 16, 2024
@Christinadobrzyn
Copy link
Contributor

Thanks @rayane-djouah and @bernhardoj - if this is a regression of #48552 should someone on the PR provide a proposal (I'm not sure)

@rayane-djouah
Copy link
Contributor

Confirmed that #48552 is the cause (specifically, this commit: 1eaeb44).

Before #48552 was merged:

Screenshot

Simulator Screenshot - iPhone 15 Pro Max - 2024-09-17 at 22 48 05

After #48552 was merged:

Screenshot

Simulator Screenshot - iPhone 15 Pro Max - 2024-09-17 at 22 49 04

When we apply styles.flex1 (like some proposals suggested) to allow the task title text to wrap onto multiple lines:

Screenshot

Simulator Screenshot - iPhone 15 Pro Max - 2024-09-17 at 22 49 57

@Expensify/design Could you please review the screenshot above and suggest the best approach for aligning the task preview elements? Should everything, including the arrow, be centered?

In the task details page, we do not center the task title, checkbox, and right arrow:

Screenshot

Simulator Screenshot - iPhone 15 Pro Max - 2024-09-17 at 22 56 19

What can we do to maintain consistent alignment across the task preview and the task details pages?

@bernhardoj
Copy link
Contributor

The PR was reverted because of #50209. In that issue, the task name is not updated dynamically.

The root cause is, the react-compiler automatically memoize the task title here. It will only be updated if taskReportID or action?.childReportName is updated.

const taskTitle = Str.htmlEncode(TaskUtils.getTaskTitle(taskReportID, action?.childReportName ?? ''));

We don't touch the task title code, but why this happens after our PR?

Before the PR, react-compiler somehow wrongly optimized the component by causing the task title to be recomputed on every re-render. So, even if we write:

const taskTitle = useMemo(() => Str.htmlEncode(TaskUtils.getTaskTitle(taskReportID, action?.childReportName ?? '')), []); // empty deps

the memo will still run on every re-render. If we disable the react-compiler for that component by using "use no memo" directive, then the useMemo will work normally again.

After the PR, react-compiler optimization now works as expected.

The new PR revert the "revert" and add the fix for above issue by passing the taskReport instead of its ID.

cc: @rayane-djouah

@rayane-djouah
Copy link
Contributor

@bernhardoj Thanks for the RCA

@rayane-djouah
Copy link
Contributor

Update: PR #50281 in review

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 7, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-10-11] [$250] Task - Long Task title is overlapped by task arrow [HOLD for payment 2024-10-14] [HOLD for payment 2024-10-11] [$250] Task - Long Task title is overlapped by task arrow Oct 7, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 7, 2024
Copy link

melvin-bot bot commented Oct 7, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Oct 7, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.45-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-14. 🎊

For reference, here are some details about the assignees on this issue:

This comment was marked as duplicate.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 9, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-10-14] [HOLD for payment 2024-10-11] [$250] Task - Long Task title is overlapped by task arrow [HOLD for payment 2024-10-16] [HOLD for payment 2024-10-14] [HOLD for payment 2024-10-11] [$250] Task - Long Task title is overlapped by task arrow Oct 9, 2024
Copy link

melvin-bot bot commented Oct 9, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.46-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-16. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Oct 9, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@rayane-djouah] The PR that introduced the bug has been identified. Link to the PR:
  • [@rayane-djouah] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@rayane-djouah] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@rayane-djouah] Determine if we should create a regression test for this bug.
  • [@rayane-djouah] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@Christinadobrzyn] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/436505

@rayane-djouah
Copy link
Contributor

BugZero Checklist

  • The PR that introduced the bug has been identified. Link to the PR: chore: update task preview UI #48552
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/48552/files#r1790970146
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • Determine if we should create a regression test for this bug. Yes
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  1. Create a task with a very long title and assign it to yourself.
  2. Verify that the task title does not overflow.
  3. Verify that the checkbox, green dot, and arrow are vertically centered relative to the avatar.

Do we agree 👍 or 👎

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 11, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Oct 14, 2024

A few more days until payment but creating a summary in preparation

Payouts due:

@bernhardoj
Copy link
Contributor

Requested in ND.

@olya-ivanochko
Copy link

There was regression

Copy link

melvin-bot bot commented Oct 14, 2024

📣 @olya-ivanochko! 📣
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.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@Christinadobrzyn
Copy link
Contributor

Hum, @rayane-djouah @bernhardoj can you confirm if this is a regression? #50209

Or what needs to be resolved before payment?

@bernhardoj
Copy link
Contributor

I wouldn't call it a regression since the root cause of the issue exists before #50209 which I explained here.

cc: @arosiclair

@JmillsExpensify
Copy link

$250 approved for @bernhardoj (agree with the comment above)

@arosiclair
Copy link
Contributor

Agree as well

@Christinadobrzyn
Copy link
Contributor

okay thanks @bernhardoj and @arosiclair!

Paid out based on this payment summary - #49086 (comment)
Regression test - https://github.com/Expensify/Expensify/issues/436505

Closing as complete!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Design External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests