[HOLD for payment 2023-03-13] [$1000] No spacing/margins above and below a Codeblock

kavimuru commented 1 year ago

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:

Open a chat
Send one message - a normal word/sentence, then space and send a codeblock. Then space and send another normal word/sentence.

Expected Result:

There should be space between the normal messages and codeblock.

Actual Result:

There's no spacing around the codeblock.

However, we want an 8 px margin above and below a codeblock.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ x] Android / native
[ x] Android / Chrome
[x ] iOS / native
[x ] iOS / Safari
[x ] MacOS / Chrome / Safari
[ x] MacOS / Desktop

Version Number: 1.2.74-0 Reproducible in staging?: y Reproducible in production?: y 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 Notes/Photos/Videos:

Expensify/Expensify Issue URL: Issue reported by: @cead22 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1676670453605579

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~018d44cb79f9fd71df
Upwork Job ID: 1630342851487846400
Last Price Increase: 2023-02-27

MelvinBot commented 1 year ago

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[x] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[x] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

jliexpensify commented 1 year ago

Posted a comment here - https://expensify.slack.com/archives/C049HHMV9SM/p1676957142442749?thread_ts=1676670453.605579&cid=C049HHMV9SM

jliexpensify commented 1 year ago

Slack comment here from Bondy saying it's not related to his PR. Back to the drawing board!

jliexpensify commented 1 year ago

Alright, I've identified a previous issue and PR (Slack comment). Asking about the next steps in-thread.

jliexpensify commented 1 year ago

Still digging in - https://expensify.slack.com/archives/C049HHMV9SM/p1677452918315599?thread_ts=1676670453.605579&cid=C049HHMV9SM

jliexpensify commented 1 year ago

Alright, this is not a regression. Moving to External!

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~018d44cb79f9fd71df

MelvinBot commented 1 year ago

Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

daraksha-dk commented 1 year ago

Proposal

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

No spacing/margins above and below a Codeblock

What is the root cause of that problem?

We aren't not defining any margin for the codeblock message which is resulting in this problem.

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

We can fix this by setting marginTop & marginBottom to some value like 8px here

What alternative solutions did you explore? (Optional)

MelvinBot commented 1 year ago

📣 @daraksha-dk! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

allroundexperts commented 1 year ago

Proposal

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

Missing spacing above and below the code block.

What is the root cause of that problem?

The root cause for this is missing margins for the code block.

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

We have to carefully add the margins in order to preserve the tab behaviour. Adding it here will sort of break the tab selection (screen shot attached).

We should add the margins here instead. This will preserve the default tab behaviour that we have right now.

Works well after the fix.

What alternative solutions did you explore? (Optional)

None

daraksha-dk commented 1 year ago

We have to carefully add the margins in order to preserve the tab behaviour. Adding it here will sort of break the tab selection

I believe displaying outlines for messages is itself a bug, and it's not intentional to show them only for codeblocks within a message. There is no reason to use an outline here as those items are not actionable/interactable. However, since this is ultimately an accessibility issue, we are currently putting it on hold until we can address it properly.

For reference:

Production (NewDot)

https://user-images.githubusercontent.com/71884990/221757726-204c8461-0aba-40f9-b771-fc81fdeabf51.mp4

Slack's treatment

https://user-images.githubusercontent.com/71884990/221757934-4ab8c093-95ee-44f6-81f6-d5ef9088bb5b.mp4

daraksha-dk commented 1 year ago

Contributor details Your Expensify account email: daraksha.dk001@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01e4a575df78cb91c6

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

allroundexperts commented 1 year ago

We have to carefully add the margins in order to preserve the tab behaviour. Adding it here will sort of break the tab selection

I believe displaying outlines for messages is itself a bug, and it's not intentional to show them only for codeblocks within a message. There is no reason to use an outline here as those items are not actionable/interactable. However, since this is ultimately an accessibility issue, we are currently putting it on hold until we can address it properly.

For reference:

Production (NewDot)

Slack's treatment

Thank you for your deep insight @daraksha-dk!

I think this is needed because on mobile, we're making code blocks pressable.

daraksha-dk commented 1 year ago

I think this is needed because on mobile, we're making code blocks pressable.

@allroundexperts - I'm unsure if this is what was intended here for platforms other than Mobile. As you can see, we swapped out the "Pressable" component with "TouchableWithoutFeedback" to eliminate the cursor here in this issue. Therefore, it wouldn't be logical to have an outline here.

Let me know your thoughts.

allroundexperts commented 1 year ago

@daraksha-dk That's a good point that you've raised. In my opinion, with the margin applied inside TouchableWithoutFeedback, the clickable area of the code block will extend beyond its border which would be a visual bug.

daraksha-dk commented 1 year ago

the clickable area of the code block will extend beyond its border which would be a visual bug.

Would it be possible for you to demonstrate this here

allroundexperts commented 1 year ago

@daraksha-dk Sure. I've attached a couple of videos. The first one is with margin applied inside the Touchable component. Notice that if I click outside the border (but with the margin), the whole message does not get highlighted before the menu opening. If we put the margin at top, long pressing above the code block border highlights the whole message properly.

https://user-images.githubusercontent.com/30054992/221892534-bcba9b5c-17e7-4a0d-a367-df08b40e490e.mov

https://user-images.githubusercontent.com/30054992/221892543-51803354-5612-43e5-99ab-39ff790c75a6.mov

allroundexperts commented 1 year ago

@daraksha-dk Another example on desktop. If you long click the code block, on desktop, the context menu opens up. If you long press on the rest of the message nothing happens. Here's a comparison of both approaches on Desktop:

https://user-images.githubusercontent.com/30054992/221895012-783f7850-1fee-4ab8-890a-a8738d150c20.mov

https://user-images.githubusercontent.com/30054992/221895045-0ad285ff-1bf2-4bf2-a17e-28340e924b3a.mov

daraksha-dk commented 1 year ago

@allroundexperts - Thank you for sharing these videos! Based on this, it seems like placing the margin at the top of the component rather than inside it will avoid this concern.

eVoloshchak commented 1 year ago

@daraksha-dk, @allroundexperts, thank you for the thorough testing @allroundexperts's proposal looks good to me, we indeed shouldn't break tab selection. Why are we adding styles to BasePreRenderer, shouldn't code be rendered by CodeRenderer?

allroundexperts commented 1 year ago

@daraksha-dk, @allroundexperts, thank you for the thorough testing @allroundexperts's proposal looks good to me, we indeed shouldn't break tab selection. Why are we adding styles to BasePreRenderer, shouldn't code be rendered by CodeRenderer?

@eVoloshchak Thank you for considering my proposal. CodeRenderer is using the InlineCodeBlock component which does not expose any style property on the container. It just has the textStyle property which takes effect somewhere deep. We can also expose a containerStyles property in BasePreRenderer and add it in CodeRenderer if this works for you.

eVoloshchak commented 1 year ago

We can also expose a containerStyles property in BasePreRenderer and add it in CodeRenderer if this works for you.

Let's do that, I think having code styles in CodeRenderer is better for readability @allroundexperts proposal looks good to me 🎀👀🎀 C+ reviewed! cc: @mountiny

MelvinBot commented 1 year ago

📣 @allroundexperts You have been assigned to this job by @mountiny! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

mountiny commented 1 year ago

Thanks @eVoloshchak for the review

thanks also to @daraksha-dk for the proposal but going with @allroundexperts feel free to raise a PR! Thanks!

allroundexperts commented 1 year ago

PR created. https://github.com/Expensify/App/pull/15570

jliexpensify commented 1 year ago

Hired @eVoloshchak and @allroundexperts

mountiny commented 1 year ago

@jliexpensify merged, this should be available for the bonus if no regression

MelvinBot commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.78-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/15570

If no regressions arise, payment will be issued on 2023-03-13. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

[x] External issue reporter
[x] Contributor that fixed the issue
[x] Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

MelvinBot commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[x] [@eVoloshchak / @mountiny] The PR that introduced the bug has been identified. Link to the PR: Judjing by https://github.com/Expensify/App/issues/14261#issuecomment-1403903811 comment this was fixed by https://github.com/Expensify/App/pull/14360. I've tested that PR, but it still isn't fixed. I think that this https://github.com/Expensify/App/issues/14261#issuecomment-1403903811 contains wrong testing steps. The issue appears if you have a single message that contains both plain text and code. I might be wrong, but on the image code blocks and plain text are on separate messages. I've also tested a couple of old commits that change BasePreRenderer, the issue still persisted, so it seems like that's how it was implemented initially.
[x] [@eVoloshchak / @mountiny] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[x] [@eVoloshchak / @mountiny] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[x] [@allroundexperts] Propose regression test steps to ensure the same bug will not reach production again.
[x] [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/269850

allroundexperts commented 1 year ago

Regression Test Proposal

Go to https://staging.new.expensify.com/ and open any chat.
Enter any comment such that the first line is plain text, second line is a code block and third line is plain text again.
Verify that equal amount of margin appears between the lines.

Do we agree 👍 or 👎

eVoloshchak commented 1 year ago

The PR that introduced the bug has been identified. Link to the PR: Judjing by this comment this was fixed by https://github.com/Expensify/App/pull/14360. I've tested that PR, but it still isn't fixed. I think that this image contains wrong testing steps. The issue appears if you have a single message that contains both plain text and code. I might be wrong, but on the image code blocks and plain text are on separate messages. I've also tested a couple of old commits that change BasePreRenderer, the issue still persisted, so it seems like that's how it was implemented initially.
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: this is a visual bug that could be caught during review, I don't think there are additional checklist items we should add

jliexpensify commented 1 year ago

Sorry, I was at a conference the last 3 days - will look at payments now!

jliexpensify commented 1 year ago

Paid @allroundexperts and @eVoloshchak

jliexpensify commented 1 year ago

I'll get @mountiny to cast his eyes on https://github.com/Expensify/App/issues/15299#issuecomment-1467083970 and I'll whip up that GH

mountiny commented 1 year ago

@jliexpensify I think that looks good, but I also think we should include a screenshot to the steps if we can to make sure its easier for the tester to confirm it looks correct

jliexpensify commented 1 year ago

@allroundexperts - could you take a look at Vit's feedback and incorporate a screenshot into your Proposal? I'll get that GH sorted after that.

allroundexperts commented 1 year ago

@jliexpensify I'm not sure if I am understanding this correctly. Can you please explain what sort of screenshot you're looking for? TIA! I see that my proposal does contain a screenshot of how it looks after the fix.

mountiny commented 1 year ago

@allroundexperts sorry, I cannot see any screenshot in the comment

Basically any screenshot of how the spacing should look like correctly now would be good.

allroundexperts commented 1 year ago

@mountiny I thought you wanted that in my original issue Proposal 😅 . I've added it to my regression test proposal as well 🙏

mountiny commented 1 year ago

Ahahah, nono, int he regression steps proposal, thank you!

I think this is good to go now @jliexpensify

Expensify / App