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

[$500] Mark down - Email with strikethrough is not underlined #37299

Closed
6 tasks done
kbecciv opened this issue Feb 27, 2024 · 10 comments
Closed
6 tasks done

[$500] Mark down - Email with strikethrough is not underlined #37299

kbecciv opened this issue Feb 27, 2024 · 10 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kbecciv
Copy link

kbecciv commented Feb 27, 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: v1.4.44-0
Reproducible in staging?: y
Issue reported by: Applause - Internal Team
Issue found when executing PR: #36773

Action Performed:

  1. Go to staging.new.expensify.com.
  2. Go to any chat.
  3. Send a message containing email in strikethrough.

Expected Result:

The email with strikethrough mark down will be underlined.

Actual Result:

The email with strikethrough mark down is not underlined.
On Android app, before sending the text, the text preview shows underlined text and the underline disappears after sending the text.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6393883_1709033357784.Android_.mp4

image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01532ae9e71730650e
  • Upwork Job ID: 1762483819651248128
  • Last Price Increase: 2024-02-27
@kbecciv kbecciv added DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor labels Feb 27, 2024
Copy link

melvin-bot bot commented Feb 27, 2024

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

@melvin-bot melvin-bot bot changed the title Mark down - Email with strikethrough is not underlined [$500] Mark down - Email with strikethrough is not underlined Feb 27, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 27, 2024
Copy link

melvin-bot bot commented Feb 27, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Feb 27, 2024
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Feb 27, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@kbecciv
Copy link
Author

kbecciv commented Feb 27, 2024

We think that this bug might be related to #vip-vsb
CC @quinthar

Copy link

melvin-bot bot commented Feb 27, 2024

Triggered auto assignment to @youssef-lr (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@Krishna2323
Copy link
Contributor

Proposal from #36349 (comment)

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

Strikethrough line is white on links and emails

What is the root cause of that problem?

We already have text-decoration styles applied to a tag, so the styles from the del tag isn't applied correctly because the text-decoration-color: rgb(90, 176, 255);.

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

To apply both underline & line-through we need to wrap the TNodeChildrenRenderer with Text which will apply the styles for the line-through .

Steps:

  1. Create a new HTML engine util function named isChildOfDel which will identify if the anchor is inside del tag or not.
function isChildOfDel(tnode: TNode): boolean {
    return isChildOfNode(tnode, (node) => node.domNode?.name !== undefined && node.domNode.name.toLowerCase() === 'del');
}
  1. Check if the anchor is inside del tag or not.
const isInsideDel = HTMLEngineUtils.isChildOfDel(tnode);
  1. Wrap the TNodeChildrenRenderer with Text with correct textDecorationLine style, we also need to flatten style from prop.const flattenStyles = StyleSheet.flatten(style as TextStyle);
<Text style={[flattenStyles, {textDecorationLine: isInsideDel ? `underline` : undefined}]}>
    <TNodeChildrenRenderer tnode={tnode} />
</Text>

Result

strikeThroughColor.mp4

Alternative

Or we can only show the line-through by overriding the styles if it is inside del .

style={[parentStyle, styles.textUnderlinePositionUnder, styles.textDecorationSkipInkNone, style, isInsideDel && {textDecorationLine: 'line-through'}]}

@Krishna2323
Copy link
Contributor

I don't think this is a blocker and caused by any PR, this is the design we use till now. I we want to change I have a proposal. Applying both strikethrough & underline is suggested because Slack and Github also does the same.

@puneetlath puneetlath added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Feb 27, 2024
Copy link

melvin-bot bot commented Feb 27, 2024

Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@puneetlath
Copy link
Contributor

I agree. I don't think it's a bug, but I'll let @isabelastisser and @youssef-lr confirm before closing.

Removing the deploy blocker label for now.

@youssef-lr
Copy link
Contributor

Yeah I'm personally fine with closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

6 participants