try fix 41908

[badeggg]

Details

Related Issues

$ Expensify/App#41908
proposal: Expensify/App#41908 (comment)

Manual Tests

Test in react-native-live-markdown

1. yarn install
2. Start examples:
   • For web:
     • From project root
     • cd WebExample
     • npm run web
   • For android:
     • From project root
     • yarn example start
     • yarn example android
   • For ios:
     • From project root
     • yarn example start
     • yarn example ios
3. Input # header, verify header style ok
4. Input > quote content, verify quote style ok

Test in expensify app

0. Prepare react-native-live-markdown
   • in react-native-live-markdown project root
   • yarn install
   • npm run prepare
1. Switch to expensify app project root
2. Checkout latest main branch
3. npm install
4. Create a tmp file
5. Copy following content to tmp file:

   cd node_modules/@expensify/react-native-live-markdown
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/LICENSE ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/README.md ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/RNLiveMarkdown.podspec ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/android ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/cpp ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/ios ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/lib ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/package.json ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/parser ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/react-native.config.js ./
   cp -r /Users/zhaoxuxu/expensify/react-native-live-markdown/src ./
   

6. Edit tmp file, change /Users/zhaoxuxu/expensify to your path to react-native-live-markdown
7. bash tmp to copy built live-markdown files to expensify-app dependency
8. Start expensify-app to verify

Linked PRs

Need change package.json version of expensify app. No pr created yet, need build react-native-live-markdown by hand and copy related files to node_modules of Expensify-App to verify.

Video evidences

ios:

ios.mov

android:

android-compressed.mov

web:

web.mov

ios-expensify:

ios-expen.mov

android-expensify:

android-expen.mov

web-expensify:

web-expen-compressed.mov

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[badeggg]

I have read the CLA Document and I hereby sign the CLA

[badeggg]

recheck

[tomekzaw]

@badeggg Thanks for the PR! What's the expected behavior when there are spaces before the blockquote but the quote itself takes more than one line of text? Here's how it works 98d50fa and it doesn't look correctly:

Screen.Recording.2024-06-12.at.12.07.11.mov

I think the proposed behavior of the blockquote ribbon will be difficult to implement correctly because there are numerous edge cases, e.g. nested blockquotes.

Instead of that, how about we don't change the position of the blockquote ribbon and just place the additional spaces before the > character? This would be way easier to implement. What do you think about this approach?

[badeggg]

how about we don't change the position of the blockquote ribbon and just place the additional spaces before the > character?

Maybe consult design team about choosing what behavior?

The video you posted looks fine to me. Leading spaces that is not in blockquote line should have no effect to blockquote.

I'll recheck the code logic and behavior.

Thank you very much for reviewing my pr.

[tomekzaw]

I believe this is not the expected behavior, the leading spaces are in a different line of text than the actual blockquote although there is no newline character in between.

Maybe consult design team about choosing what behavior?

Yes, let's do that. My recommended solution would be to place the additional spaces between the blockquote border and the > character.

[tomekzaw]

cc @thienlnam @shawnborton How do you think this should behave?

[shawnborton]

Interesting... I'm not too sure what to think, so tapping the @Expensify/design team for a second opinion. Would it make sense to only use the block quote style if the > started at the left edge of the text input? And if not, just display it as regular text?

[tomekzaw]

Would it make sense to only use the block quote style if the > started at the left edge of the text input? And if not, just display it as regular text?

That's actually what we currently do and this is the exact problem reported in the original issue: Expensify/App#41908

If you type > Hello, once the message is sent, it will be trimmed to > Hello and converted into a blockquote.

Currently, the live markdown preview doesn't recognize > Hello as a blockquote which is inconsistent with the output in chat history.

Also, we can't just trim the message and not convert into a blockquote, because once the message is edited, it would be converted into a blockquote even if the actual characters did not change.

So, we still need to recognize the input > Hello as a blockquote even though there are spaces before the > character, but we need to decide how to present it.

From the technical perspective, we can't place the space characters "before" the blockquote ribbon, also this would be pretty bad UX.

So the recommended solution is to place the spaces "between" the blockquote ribbon and > character. So effectively this is just like a regular paragraph of text, just with the blockquote ribbon on the left side (good UX).

[shawnborton]

Hmm but taking a step back, what is the actual use case where a user would want to put spaces before the block quote? For example, if I type s>s we wouldn't use a block quote, right? So why should >s be any different?

[tomekzaw]

There's no use case for adding spaces before blockquote but it doesn't mean it can't happen. The user can just type >s, hit Enter and the sent message will be "s" in a blockquote. That's the expected behavior in the chat history. Now we're trying to define the expected behavior of the live markdown preview in the composer.

[shawnborton]

That's the expected behavior in the chat history.

If there is no use case for this, then why is that the expected behavior? My suggestion is also how Slack handles it too:

[dannymcclain]

So I was just trying to test these situations in Slack and Github (where I use markdown the most) and I honestly don't even know what to think anymore!

CleanShot.2024-06-13.at.09.34.31.mp4

CleanShot.2024-06-13.at.09.35.07.mp4

Hmm but taking a step back, what is the actual use case where a user would want to put spaces before the block quote? For example, if I type s>s we wouldn't use a block quote, right? So why should >s be any different?

Should we just keep this simple? What would be the simplest way to handle this?

[shawnborton]

I personally think Slack's approach is the most intuitive and simplest way to handle it.

[dannymcclain]

I personally think Slack's approach is the most intuitive and simplest way to handle it.

I'm good with that. Essentially what you see is what you get. So if you start a line with > or > we automatically style it as a block quote. Otherwise we leave it alone. So > Yo would not be formatted as a block quote. Is that an accurate summary @shawnborton?

[shawnborton]

That's what I would expect, yup!

[badeggg]

So > Yo would not be formatted as a block quote.

@dannymcclain @shawnborton Hello guys. This behavior is actually what expensify-app currently doing when the comment is in composer box, the problem is we will trim leading and trailing spaces when hit enter key. So the comment will be formatted as block quote after being sent, while not being formatted as block quote in composer box. This seems not fine.

[shawnborton]

Why would we format it as a block quote after it is sent? Why not just format it like this:

[shawnborton]

I thought the whole point of live markdown is that it's basically WYSIWYG - if we don't show the blockquote in the composer, let's not show it when the message is sent either.

[badeggg]

Should we not trim leading and trailing spaces when sending, if the comment content includes non-spaces chars?
I guess the initial reason of 'trim leading and trailing spaces' is that we don't want all-spaces-comment.

[badeggg]

Why would we format it as a block quote after it is sent? Why not just format it like this

Make sense to me

[shawnborton]

I'm not saying to not trim the spaces. Slack does indeed trim the spaces before hand. But it just doesn't turn it into a blockquote - it trims the spaces and then just renders the > as normal text:

CleanShot.2024-06-13.at.17.21.19.mp4

[thienlnam]

That's actually what we currently do and this is the exact problem reported in the original issue: Expensify/App#41908

If you type > Hello, once the message is sent, it will be trimmed to > Hello and converted into a blockquote.

Currently, the live markdown preview doesn't recognize > Hello as a blockquote which is inconsistent with the output in chat history.

Additionally, this is against our principle of not modifying the user input. I agree with others about handling it the same way slack does - if it has pre-pended spaces it will show as regular text

[tomekzaw]

@thienlnam @shawnborton

I agree with others about handling it the same way slack does - if it has pre-pended spaces it will show as regular text

This sounds good but it breaks when you consider that the message can be edited. Here's the case:

Assume that the user types: " > Hello" (where represents leading spaces)

According to your idea, let's assume that we don't convert this message into a blockquote, we just trim all the leading spaces.

Sent message is: "> Hello"

Now, the user edits this message. It starts from ">" and there are no leading spaces so it is recognized by Live Markdown as a blockquote which is inconsistent with the sent message.

The user just hits Enter without making any changes. The edited message is a blockquote so it's different than the original message, even though the user did not make any changes.

When we trim the spaces before the blockquote, we lose the context. Slack stores messages as rich text so the formatting is an additional context on top of the actual text. On Slack, you can send a message "Hello world" with no asterisks and still get bold formatting.

In Expensify, the messages are live markdown formatted, so there is no additional data apart from the actual text. The formatting is implied from the actual text so if the message starts with a ">", it is a blockquote. There's no way to tell that it's not a blockquote. There's no way that the same string will be recognized as two different possible formattings:

Case #1: "> Hello" (as a regular string)
Case #2: "> Hello" (as a blockquote)

(Actually, it's possible to distinguish these two cases in HTML which Expensify uses in the database, but the user types the message in Markdown and the messages are converted HTML->MD when they are edited and MD->HTML when they are being sent).

@badeggg is right in this comment: #382 (comment)

[shawnborton]

Now, the user edits this message. It starts from ">" and there are no leading spaces so it is recognized by Live Markdown as a blockquote which is inconsistent with the sent message.

The user just hits Enter without making any changes. The edited message is a blockquote so it's different than the original message, even though the user did not make any changes.

Let's just prevent that case from happening.

There's no way to tell that it's not a blockquote.

I find this a bit hard to believe personally. You are telling me that there is absolutely no way we could detect that if a user was editing a message that already started with a plain > character (no blockquote), that we couldn't prevent the saved edit from turning into a block quote?

[thienlnam]

trim leading and trailing spaces

My proposal is that we just stop doing this since that is modifying the user input. Granted, there might be other concerns with doing this (allowing a bunch of whitespace before and after any text is sent) but it seems like that would solve the other problems here

[badeggg]

Agree with @thienlnam , maybe we should not trim spaces anymore, at least not trim leading spaces. Although we still need check all-spaces-comment case.

[luacmartins]

@badeggg ts is failing and there are conflicts

[badeggg]

@luacmartins Hi, I think we should decide the expected behavior first. Seems the code changes I made is not the wanted behavior. Can you check comment history in this pr and help make a decision?

[luacmartins]

WRT this comment, we can't just prevent it from happening for the reasons @tomekzaw described here. We can certainly prevent the no edits case, but if the user edited the message, let's say to add a . at the end, the message would be formatted from not having a blockquote to having one.

I find this a bit hard to believe personally. You are telling me that there is absolutely no way we could detect that if a user was editing a message that already started with a plain > character (no blockquote), that we couldn't prevent the saved edit from turning into a block quote?

As @tomekzaw pointed out, we store the messages as plain text and html. We cannot differentiate them from plain text alone, we'd have to use the html but that comes with its own set of problems as @tomekzaw also pointed out since the user types the message in markdown, not html.

I agree with @thienlnam. I think we should stop trimming spaces from messages

[thienlnam]

Okay, to go with that solution we'll need to do a couple things

• Prevent stripping the message in the FE
• (Internal) Prevent trimming the message in the BE

@badeggg Could you please update this PR or just create a new one with our new agreed solution, and then I will spin up a PR for you to test with?

[luacmartins]

Agree on the steps above.

[badeggg]

Ok, I will check how to prevent stripping the message in the FE tomorrow. Seems we need change Expensify-App code to prevent trimming, I'll need start a new pr.

[badeggg]

@thienlnam @luacmartins and every one here, I have made a new pr: Expensify/App#44060

[tomekzaw]

Can we close this PR then? @badeggg

[badeggg]

Ok, closing