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

Chat - Preview shows | > | > | when adding multiple > in the composer #36227

Closed
3 of 6 tasks
lanitochka17 opened this issue Feb 9, 2024 · 19 comments
Closed
3 of 6 tasks
Assignees

Comments

@lanitochka17
Copy link

lanitochka17 commented Feb 9, 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-39-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to any chat
  3. Type >>>>> in the composer

Expected Result:

The composer displays | >>> (Android and iOS app behavior)

  1. Limit max depth of the quote inside ExpensiMark
  2. change implementation of blockquotes in chat to show multi-level quotes
  3. Enable inside react-native-live-markdown library support for multilevel quotes on all platforms - here won't be any limit, as parser should handle it.

Discussion for expected result https://expensify.slack.com/archives/C01GTK53T8Q/p1707904106957409

Actual Result:

The composer shows | > | > | instead of | >>>>>

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

Bug6373593_1707485081273.bandicam_2024-02-09_13-06-26-947__1_.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Feb 9, 2024
Copy link
Contributor

github-actions bot commented Feb 9, 2024

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

Copy link

melvin-bot bot commented Feb 9, 2024

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

@lanitochka17
Copy link
Author

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

@shubham1206agra
Copy link
Contributor

@thienlnam @tomekzaw I think we jumped the gun here as we don't support multi-level blockquote yet. I think we need to implement these in ExpensiMark.

@lakchote
Copy link
Contributor

lakchote commented Feb 9, 2024

Offending PR found. This is the one related to markdown preview.

cc @shubham1206agra @tomekzaw @thienlnam

@tomekzaw
Copy link
Contributor

tomekzaw commented Feb 9, 2024

Thanks for reporting this, we will decide on the expected behavior here and implement it. Please assign me

@thienlnam
Copy link
Contributor

We've reverted the offending PR #35837, so this is no longer a blocker

@thienlnam thienlnam added Weekly KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Feb 9, 2024
@thienlnam thienlnam self-assigned this Feb 9, 2024
@thienlnam
Copy link
Contributor

Discussion for expected outcome here: https://expensify.slack.com/archives/C01GTK53T8Q/p1707904106957409
The new plan from @robertKozik:

  1. Limit max depth of the quote inside ExpensiMark
  2. change implementation of blockquotes in chat to show multi-level quotes
  3. Enable inside react-native-live-markdown library support for multilevel quotes on all platforms - here won't be any limit, as parser should handle it.

@robertKozik
Copy link
Contributor

Hi! Small update on my end.
I've managed to create draft PR with ExpensiMark changes (Here: Expensify/expensify-common#654) I've come across the undiscovered issue, that parser was removing one excess > every time - it slowed me down a little, but I've managed to fix it as well.

Now I'm on adjusting react-native-live-markdown, as I think should be merged before, and after them we should update E/App

@robertKozik
Copy link
Contributor

The support for multiple quotes has already been merged into the react-native-live-markdown library.

The PR within expensify-common is ready for review, and once merged, it will be the final piece needed to fully support nested quotes.

@lakchote
Copy link
Contributor

lakchote commented Mar 4, 2024

Any update @robertKozik? The PR in expensify-common has been merged.

@melvin-bot melvin-bot bot removed the Overdue label Mar 4, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 12, 2024
@thienlnam
Copy link
Contributor

Bump on this - looks like we just need to bump a version or something?

@melvin-bot melvin-bot bot removed the Overdue label Mar 12, 2024
@robertKozik
Copy link
Contributor

Yeah, sorry for my absence in this thread. New version of the expensify-common is already merged in the live-markdown library. Version bump for E/App will take place inside the PR with new version of react-native-live-markdown for the App

@bernhardoj
Copy link
Contributor

Hi, I have a live markdown PR here to fix this app issue and I notice that the current version of live markdown in app is 0.1.5, but the latest version of live markdown is 0.1.28.

@robertKozik I don't see the App PR yet, so just want to confirm, is your App PR will update the live markdown version to 0.1.28?

(our plan is to wait for the live markdown version to be updated to 0.1.28 and then I will separately create another PR to update it further to include my live markdown PR above. I was thinking to ask for your help to include my live markdown PR above, but I think it would be out of scope)

@melvin-bot melvin-bot bot added the Overdue label Mar 22, 2024
@BartoszGrajdek
Copy link
Contributor

BartoszGrajdek commented Mar 28, 2024

@thienlnam I believe this is now fixed 🤔 There's still #38199 but that's a separate issue

@BartoszGrajdek
Copy link
Contributor

Okay I tested it and it still requires some more investigation, so let's keep this issue for now

@BartoszGrajdek
Copy link
Contributor

This seems to not be a problem anymore @thienlnam can we get someone to test it again? 🤔

@kavimuru
Copy link

kavimuru commented Apr 3, 2024

@thienlnam Issue is fixed.

2024-04-03.20-07-42.mp4

@thienlnam
Copy link
Contributor

We're good, thanks!

@melvin-bot melvin-bot bot removed the Overdue label Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants