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

mWeb - Chat - In offline, emojis are not striked out when deleted with markdown #42212

Closed
1 of 6 tasks
lanitochka17 opened this issue May 15, 2024 · 10 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented May 15, 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: 1.4.74
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/4562779
Issue reported by: Applause - Internal Team

Action Performed:

Pre-condition: Be in offline

  1. Go to https://staging.new.expensify.com/home
  2. Tap on a report
  3. Enter >😁
  4. Select the message
  5. Copy and paste the message twice
  6. Send the message
  7. Long press the sent message and delete it

Expected Result:

In offline, emojis must be strike out when deleted with markdown

Actual Result:

In offline, emojis are not striked out when deleted with markdown

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

Bug6481989_1715802082296.emoji.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 15, 2024
Copy link

melvin-bot bot commented May 15, 2024

Triggered auto assignment to @laurenreidexpensify (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.

@lanitochka17
Copy link
Author

@laurenreidexpensify 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

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@laurenreidexpensify
Copy link
Contributor

@Expensify/design curious for your thoughts here - this doesn't seem like a bug worth addressing ?

@bernhardoj
Copy link
Contributor

Proposal

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

The deleted message with emoji inside a markdown doesn't get strikethrough.

What is the root cause of that problem?

In #40617, we prevent the emoji to be formatted (bold, italic, strikethrough), so if we type ~😄~, the strikethrough won't apply.

...display.dInlineFlex,

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

If we want to show the strikethrough only when the message is deleted, first, convert emojiDefaultStyles to a function that acceps whether the message is deleted or not. If it's deleted, then don't apply the display: inline-flex (web) and textDecorationLine: none style (native).

const emojiDefaultStyles: EmojiDefaultStyles = {

To know whether it's deleted or not, we can add deleted attribute to the emoji tag.

const htmlWithDeletedTag = styleAsDeleted ? `<del>${html}</del>` : html;

const htmlWithDeletedTag = styleAsDeleted ? `<del>${html.replaceAll('<emoji>', '<emoji deleted>')}</del>` : html;

Then, check whether deleted exists on the attributes and pass it to the emojiDefaultStyles style.

function EmojiRenderer({tnode}: CustomRendererProps<TText | TPhrasing>) {
const styles = useThemeStyles();
const style = 'islarge' in tnode.attributes ? styles.onlyEmojisText : {};
return (
<EmojiWithTooltip
style={[style, styles.cursorDefault, styles.emojiDefaultStyles]}

styles.emojiDefaultStyles('deleted' in tnode.attributes)

We need to apply it to InlineCodeBlock and WrappedText too.

What alternative solutions did you explore? (Optional)

Create a new context to hold the deleted message value, wrap the context here,

return (
<RenderCommentHTML
source={source}
html={htmlWithTag}
/>
);

and access the context (useContext) in EmojiRenderer, InlineCodeBlock, and WrappedText.

@trjExpensify
Copy link
Contributor

@Expensify/design curious for your thoughts here - this doesn't seem like a bug worth addressing ?

Yeah, interesting one. It is "breaking the pattern" insofar as not providing feedback that it's pending to be deleted with strikethrough, but being greyed out is signalling there's a change pending to happen. I think it's fine to not apply strikethrough to the emoji in the offline case, the same as we don't allow it via markdown. We do something similar with a custom avatar you delete when offline, we don't put strikethrough through your profile pic while it's pending to be deleted - it's just greyed out because it looks weird otherwise. So I think we can close this personally 👍

@shawnborton
Copy link
Contributor

Yeah, I'm cool with that too. Feels pretty edge case as well.

@dannymcclain
Copy link
Contributor

Same here. I'm good with closing.

@bernhardoj
Copy link
Contributor

But the emoji gets strikethrough if it's plain text (not a markdown), I think it's inconsistent.

image

@trjExpensify
Copy link
Contributor

Oh cool, so it even works in the mainline case! So yeah, I still think it's fine to not worry about this. We don't strikethrough emojis in markdown, whether that's a user's input or due to a pending offline action. Equally though, in a more standard case of words with emojis in quoted markdown, I think this looks fine for the case we're talking about:

image

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
Projects
None yet
Development

No branches or pull requests

6 participants