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

[HOLD for payment 2022-05-24][$1000] Native Apps - There is an extra space above codeblocks in android and IOS - reported by @sobitneupane #8089

Closed
mvtglobally opened this issue Mar 11, 2022 · 48 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@mvtglobally
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Send codeblock with message above and below:
    Here is come code
    const myLineOfCode = 'some code';
    

Expected Result:

Same output in all platforms.

Actual Result:

Extra space above codeblocks in android and IOS.

Workaround:

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

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.1.41-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Related to #7496 (comment)

Screenshot_1644495649
Screen Shot 2022-02-10 at 18 04 02

Expensify/Expensify Issue URL:
Issue reported by: @sobitneupane
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644507497800049

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @MonilBhavsar (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@MonilBhavsar
Copy link
Contributor

Looks like <Text> component is occupying more than half of height of <ReportActionItemGrouped> component

Screenshot 2022-03-14 at 9 44 26 PM

Not blocking anything and occurs on iOS/Android, so low priority

@MonilBhavsar MonilBhavsar added Improvement Item broken or needs improvement. External Added to denote the issue can be worked on by a contributor labels Mar 14, 2022
@MelvinBot
Copy link

Triggered auto assignment to @JmillsExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@JmillsExpensify
Copy link

Agreed, not an incredible priority. I'll go ahead and create an Upwork job because I do agree we should polish. Job is here: https://www.upwork.com/jobs/~0127315d24f0e23219

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 15, 2022
@MelvinBot
Copy link

Triggered auto assignment to @marcochavezf (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@JmillsExpensify
Copy link

Still waiting for volunteers. Increased the Upwork job to $500 per the process.

@JmillsExpensify
Copy link

JmillsExpensify commented Mar 30, 2022

Still waiting on volunteers. Increasing the price to $1000, which is ironic given this isn't incredibly pressing, but I'd like to stay true to the process here.

@JmillsExpensify JmillsExpensify changed the title Native Apps - There is an extra space above codeblocks in android and IOS - reported by @sobitneupane [$1000] Native Apps - There is an extra space above codeblocks in android and IOS - reported by @sobitneupane Mar 30, 2022
@aneequeahmad
Copy link
Contributor

aneequeahmad commented Apr 7, 2022

PROPOSAL

CAUSE OF ISSUE

Converting the comment from MD into HTML in addAction function(Report.js) is causing the issue. ExpensiMark.js replace function is called with message text that applies rules to text to ensure that any Html the user puts into the comment field shows as raw Html. In the rules below rule

 {
                // when <pre> and <br /> occur together extra line is added so removing <br />
                name: 'replacepre',
                regex: /<\/pre>\s*<br\s*[/]?>/gi,
                replacement: '</pre>',
     },
     

is not replacing <br /><pre> with <pre> to remove extra line added but only removes <pre>

BEST POSSIBLE SOLUTION

This rule when added

     {
                //  when <br /> and <pre> occur together extra line is added remove <br />
                name: 'replacebr',
                regex: /<br\s*[\/]?><pre>\s*/gi,  
                replacement: '<pre>'
            }

replaces <br /><pre> with <pre> which fixes the issue.

TO BE NOTED AND NEED SUGGESTION
Also this regex /<\/pre>\s*<br\s*[/]?>/gi doesn't seem to be valid so replacing it with /<\/pre>\s*<br\s*[\/]?>/gi to make this regex valid.

ScreenShot is attached after the fix is applied.

Screen Shot 2022-04-07 at 5 00 45 AM

@melvin-bot melvin-bot bot added the Overdue label Apr 7, 2022
@JmillsExpensify
Copy link

@rushatgabhane Circling back on this one because we owe feedback for this proposal. Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Apr 13, 2022
@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2022
@marcochavezf marcochavezf added the Reviewing Has a PR in review label Apr 29, 2022
@melvin-bot

This comment was marked as off-topic.

@JmillsExpensify
Copy link

Thanks @marcochavezf! Looks like we have a couple of updates in the linked PR.

@aneequeahmad
Copy link
Contributor

@JmillsExpensify, The Upwork job is no longer available for some reason. PR is merged and PR for App is raised as well. Thanks

@JmillsExpensify
Copy link

No worries. I've got you covered here: https://www.upwork.com/jobs/~012ed1a030f60ce0f5. Can you apply? Then we can issue payment once the standard regression period ends.

@JmillsExpensify
Copy link

cc @sobitneupane and @rushatgabhane as well for the new job posting.

@aneequeahmad
Copy link
Contributor

@JmillsExpensify Applied, yeah sounds good let's wait for the regression period. Thanks for your message.

@JmillsExpensify
Copy link

Still working through considerations in the last linked PR.

@JmillsExpensify
Copy link

Working through issues in the linked PR to get the project running. No progress since last week.

@rushatgabhane
Copy link
Member

rushatgabhane commented May 20, 2022

Interesting..

Looks like @aneequeahmad's changes to expensify-common hit production 3 days ago because of an another PR #8948, which updated expensify-common to a more up-to-date version.

If @marcochavezf agrees with me as well, we can ask the QA team to test this issue and close it out in 7 days.

@marcochavezf
Copy link
Contributor

Yeah, it's interesting that expensify-common was eventually updated in E/App because of another PR. Thanks for testing again on Android @rushatgabhane!

If @marcochavezf agrees with me as well, we can ask the QA team to test this issue and close it out in 7 days.

Sure, sounds like a good plan to me! I already sent a message to the QA team to test again.

@mvtglobally
Copy link
Author

@marcochavezf
Can you confirm the build to test this on?

@rushatgabhane
Copy link
Member

rushatgabhane commented May 20, 2022

@mvtglobally production build on mobile platforms.

@JmillsExpensify
Copy link

Thanks everyone! So based on when #8948 hit production, today is the end of the regression period. Accordingly, I'm holding to hold for the next QA test. Assuming that is happening sometime today, so @mvtglobally it'd be great if you can confirm when this passes. I'll go ahead and put a hold for payment tomorrow.

@JmillsExpensify JmillsExpensify changed the title [$1000] Native Apps - There is an extra space above codeblocks in android and IOS - reported by @sobitneupane [HOLD for payment 2022-05-24][$1000] Native Apps - There is an extra space above codeblocks in android and IOS - reported by @sobitneupane May 23, 2022
@mvtglobally
Copy link
Author

@JmillsExpensify @rushatgabhane
It seems like there is still some gap and issue is repro. See below
ios_8089-2105

@rushatgabhane
Copy link
Member

rushatgabhane commented May 24, 2022

@mvtglobally all those look like they're different messages.

Can we try that again with a single message and see if it works?

Screenshot_20220524-081518

@aneequeahmad
Copy link
Contributor

@JmillsExpensify The Upwork job is no longer available, also i haven't been hired for this job.
Could i ask when the payment will be issued ? I'm unaware of the process as this is my first job. Thanks

@rushatgabhane
Copy link
Member

@aneequeahmad the job might have expired and a new one will be created.

Could i ask when the payment will be issued

Once the issue is not reproducible by the QA team, the payments will be settled.
Thanks for understanding!

@aneequeahmad
Copy link
Contributor

@rushatgabhane Ahhh alright! thank you for your comment 🥇

@JmillsExpensify
Copy link

JmillsExpensify commented May 24, 2022

@JmillsExpensify The Upwork job is no longer available, also i haven't been hired for this job. Could i ask when the payment will be issued ? I'm unaware of the process as this is my first job. Thanks

You're good! You already applied to the existing job post here: https://www.upwork.com/jobs/~012ed1a030f60ce0f5. I've hired you, same for @rushatgabhane as C+ and @sobitneupane for reporting. Thanks all, let me know we still have any issues.

@JmillsExpensify
Copy link

@mvtglobally I just tested this on the latest build and I think we're good. Would you mind confirming so we can close this out in the next QA round? Thanks!
IMG_FE9425A837C0-1

@mvtglobally
Copy link
Author

@JmillsExpensify @rushatgabhane
I think it's looking good image

@rushatgabhane
Copy link
Member

Fantastic, thanks everyone!

@JmillsExpensify
Copy link

Ok great! I've just issued payments to all contributors, so I'm closing the issue. Don't hesitate to follow-up with any questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants