search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.83k forks source link

[$500] Android/iOS - Code blocks are overflowing the app border #27913

Open zanyrenney opened 1 year ago

zanyrenney commented 1 year ago

Issue is failing https://github.com/Expensify/App/pull/4624 (CC @parasharrajat)

Action Performed:

  1. Navigate to a conversation in iOS or Android
  2. Send a long message in a code block

Expected Result:

Code block should be displayed in the area of the conversation.

Actual Result:

Code block is partially visible because is overflowing the app border.

Platform:

Where is this issue occurring?

Version Number: 1.0.86-2

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ecab5a591272dd78
  • Upwork Job ID: 1602379910299570176
  • Last Price Increase: 2023-10-19
Issue OwnerCurrent Issue Owner: @puneetlath
zanyrenney commented 8 months ago

Awesome work @cubuspl42 please link the PRs when you do split them up here for tracking.

cubuspl42 commented 8 months ago

We have a new, proper tracking issue: https://github.com/facebook/react-native/issues/42602

It contains all the current status, including references to the new PRs.

zanyrenney commented 8 months ago

thanks @cubuspl42

zanyrenney commented 8 months ago

any further update here?

cubuspl42 commented 8 months ago

The Wave 3 is not going too well. My intention was to submit smaller changes first, so I don't get accused of submitting too big, "unreviewable" chunks, but we're kind of stuck on the review, arguing about irrelevant things. I'll submit new PRs this week and likely close two from Wave 3.

Overall, things are not perfect, but the situation is still not hopeless.

cubuspl42 commented 8 months ago

The current count of merged PRs is 8

zanyrenney commented 7 months ago

Thanks for the update, @cubuspl42 anything further?

cubuspl42 commented 7 months ago

After a series of "prep up" PRs, I submitted a bigger PR implementing the "span" feature on Android:

I'm awaiting high-level feedback. So far, pings are being ignored.

roryabraham commented 7 months ago

We got some high-level feedback from Meta. It's a bit unfortunate, but I think it's valid nonetheless. Brought this up in slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1709140390579929

parasharrajat commented 7 months ago

Ah....That is bad news.

cubuspl42 commented 7 months ago

I replied on Slack. I think we can manage to do it on our own. It will be more complex, but likely possible.

parasharrajat commented 7 months ago

I am in the same boat as you.

zanyrenney commented 7 months ago

has there been any further feedback since the feedback with meta and related slack post? @cubuspl42 @parasharrajat @roryabraham

roryabraham commented 7 months ago

Not really, and I think we kind of hit a standstill in that thread without a clear consensus on a plan of action.

cubuspl42 commented 7 months ago

has there been any further feedback since the feedback with meta and related slack post?

There has been some more information exchange here.

Summing up:

But honestly, I would recommend catching up with the whole thread I linked (@roryabraham, @puneetlath).

we kind of hit a standstill in that thread without a clear consensus on a plan of action.

I think we have some project leadership challenges here. I it was up to me, I would completely pivot to implementing the external Text-alike component.

Ways forward (as mentioned on Slack):

Not ways forward:

@puneetlath Maybe there's some potential for you to step in, as you're the assigned internal engineer?

parasharrajat commented 7 months ago

I think at this stage, we should pause and plan the future roadmap. I agree creating POCs before implementation will help us here. A lot of effort has been put into this implementation by @cubuspl42. This is the second attempt for this feature. Looks like some ideas are flowing on the Slack thread for a different implementation technique. It would be good to consider those as well.

@cubuspl42 What do you think of how far we are if we continue pursuing your idea? I know that it has been a challenging experience for you. Do you have second thoughts about the task? Do you suggest assigning it to someone else to analyze possible solutions?

Caveats

  1. We don't want to fork the RN repo. So we either implement this on RN or create a patch. Implementing this on RNs is found to be very challenging.
cubuspl42 commented 7 months ago

We could try implementing this on the forked RN version and see if we reach the end solution then we could create a patch for it.

Yes, if the end goal is to merge this into upstream, this is the path we should switch to.

What do you think of how far we are if we continue pursuing your idea?

In terms of the implementation? Depends.

C++ / Fabric: basically done.

Android / Fabric: very far.

iOS / Fabric: early phase, but the implementation would be analogical

Paper: not started

Do you have second thoughts about the task?

I'm having second thoughts about whether I'm willing to continue pushing it upstream. In the beginning, I treated this like a challenge I'm up for. Now, I'm close to believing that this might be pragmatically impossible. I'm unsure whether there's a precedence of the React Native team accepting such a large and low-level feature directly from the community.

If we decide that the endgame is having this merged upstream, I might prefer to hand this over or re-discuss the arrangements.

parasharrajat commented 7 months ago

Thanks for your input.

parasharrajat commented 6 months ago

@puneetlath Could you please help us push this forward? We need to work on action plan here. How can I help?

zanyrenney commented 5 months ago

bump @puneetlath are you able to help with what an action plan for this could be, please?

puneetlath commented 5 months ago

I had talked before with @tomekzaw about potentially helping out with this as it seems like it could be in his wheelhouse. @tomekzaw is this still something you'd be able to help with?

parasharrajat commented 4 months ago

@tomekzaw @puneetlath What is next here?

tomekzaw commented 4 months ago

Hey, this is actually related to Markdown message preview (in report history) rather than Live Markdown preview (in the composer box). We might take a look on it but generally we need to prioritize Live Markdown preview issues. Let me ask internally if we have capacity to tackle with this issue.

filip-solecki commented 4 months ago

Hi! I am Filip from SWM an expert agency and I'd like to work on this issue on Monday(as we have bank holiday tomorrow and the next day)

filip-solecki commented 4 months ago

@parasharrajat Are you able to reproduce this issue? Or anyone else? I am trying on both iOS and Android and looks like I cannot reproduce it. Or am I missing something? Videos attached below:

https://github.com/Expensify/App/assets/153545991/807bb4fd-da99-4e2c-a10e-51668d980fce

https://github.com/Expensify/App/assets/153545991/4cc0fad1-ce94-4574-be75-0b6ffd04ca69

parasharrajat commented 4 months ago

Let me check

parasharrajat commented 4 months ago

@filip-solecki I can reproduce this.

Screenshot 2024-06-04 at 8 15 48 PM

Try with this message. 

`thissadasdasdsadsadasdadsadasdasdasdasdasdasdasdasdasdasdsadsadggggggggggggggggg` multilingual 
`https://www.google.com/search?q=google&oq=goog&gs_lcrp=EgZjaHJvbWUqEAgAEAAYgwEY4wIYsQMYgAQyEAgAEAAYgwEY4wIYsQMYgAQyEwgBEC4YgwEYxwEYsQMY0QMYgAQyDQgCEAAYgwEYsQMYgAQyBggDEEUYOzIGCAQQRRg8MgYIBRBFGDwyBggGEEUYPDIGCAcQBRhA0gEHNzM1ajBqN6gCALACAA&sourceid=chrome&ie=UTF-8`
filip-solecki commented 4 months ago

On your screenshot code block is not overflowing the app border

filip-solecki commented 4 months ago

The issue right now is that the code block is not fully visible, isn't it any other bug?

parasharrajat commented 4 months ago

Yes, This is somewhat different from the original issue but the root cause is the same. We want to solve the problem where Code block data is not fully visible. It should wrap like normal text and proper formatting should be applied.

I will share the rules for inline code block which is expected.

cdOut commented 4 months ago

Hi, I'm Tymoteusz from Software Mansion, the expert agency, and I'll be taking over this issue from Filip.

@parasharrajat could you please share the inline code block rules you mentioned in the previous comment?

parasharrajat commented 4 months ago

Sure @cdOut. They are attached at the end of comment https://github.com/Expensify/App/issues/27913#issuecomment-2147729447.

parasharrajat commented 4 months ago

Here are some rules from https://github.com/Expensify/App/issues/4733#issuecomment-1313335094 .

  1. The inline code block element should be behave as normal text. i.e follow same text wrapping and layout.
  2. The inline code block should have a border around the text and grey background as already configured as well as some gap between the border.
  3. Inline code block should follow the same line-height as much as possible. Normal text takes about 18px of vertical space, we want the same for inline code block. (not sure if its 20 or 18)
  4. It should wrap a normal text will do.
  5. The paragraph containing the inline code block or the inline code block alone, should be contained within the boundary of ReportActionItem which is 20px from right edge of the screen.
  6. we should try to get a similar line spacing between lines where an inline code block is used.

You can use following for testing. 

1. This is a normal `message` with some inline `code` blocks but long `enough` to wrap a few lines.
2. This is a normal `message with a long inline code blocks but long enough` to wrap a few lines.
3. This is a `message with two long inline code blocks but long enough` to wrap a few lines. To compare with first line code block use this `message with two long inline code blocks but long enough`.
4. `Lastly we will just use only inline code block which wraps a few lines so test this out`.
5. `A short sample`.
7.  `Biiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiggggggggggggggggggggggggggggggggggggggggggggggggggzzzz`.
8.`Biiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiigggggggggggggggggggggzzzzzzz` with some raw text to compare the layout flow. 
9. Normal text above `Biiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiigggggggggggggggggggggzzzzzzz` normal text below.
10. Normal text above `[Biiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiigggggggggggggggggggggzzzzzzz](https://github.com/Expensify/App/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+user-review-requested%3A%40me)` normal text below.
11. Normal text above `https://github.com/Expensify/App/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+user-review-requested%3A%40me` normal text below.

A rendering UI sample.

image

cdOut commented 4 months ago

Awesome, thank you

parasharrajat commented 4 months ago

@zanyrenney Please change this issue to weekly.

parasharrajat commented 4 months ago

@cdOut Please keep us posted.

cdOut commented 4 months ago

Of course, here's the update @parasharrajat. I've figured out a way to properly display it on mobile devices but I'm still figuring out the way to properly style it the same way as it is on desktop. The PR for this issue should be up in draft by tomorrow.

cdOut commented 4 months ago

I've had to refocus on another issue that was more critical at the time, I'm going to continue working on this one today.

cdOut commented 3 months ago

I've found a viable solution but it does have some discrepancies between IOS and Android, currently working on making it look the same on both platforms.

parasharrajat commented 3 months ago

@cdOut Any updates?

cdOut commented 3 months ago

I'll have a draft PR on monday and will push through internal review on the same day, should be in review by the EOD in the beginning of next week.

cdOut commented 2 months ago

Sorry for the delays on this one, I had other critical issues I had to work on first. I'll focus on getting this one merged this week.

puneetlath commented 2 months ago

How's it going @cdOut?

cdOut commented 2 months ago

@puneetlath Again sorry for the delays, one of my tasks, namely Guided Setup 3, got unblocked since the backends got merged and had to prioritize it, I'm going to look into this one now.

cdOut commented 2 months ago

Since this is a pretty complicated issue I've had to rework the previous implementation for the fix, it wasn't working properly and had a delayed response to adding the correct styles. Working on a better fix by the end of this week.

puneetlath commented 1 month ago

@cdOut how is this going?

cdOut commented 1 month ago

Hi, sorry for all the delays with updates on this one, I've lost track of the issue due to working on high priority ones at that time. I'm going to pick up this issue and push a solution in the PR by the EOW.

My current solution was flawed as it had a slight delay between opening the report chat and rendering the valid code content, I'll look for a way to style the text as inline with a proper block border on native devices without having to wait for it to render first.

cdOut commented 1 month ago

I found a way to handle this case with a pretty minimal solution that doesn't rely rendering the content first, I've moved the PR into review.

melvin-bot[bot] commented 2 weeks ago

This issue has not been updated in over 15 days. @puneetlath, @parasharrajat, @zanyrenney, @cdOut eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

parasharrajat commented 2 weeks ago

Very close to merge.