Closed kavimuru closed 1 year ago
Triggered auto assignment to @trjExpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are β
)Assigning myself since I was also pulled into the linked Slack convo.
I'm able to reproduce the issue, and while a small visual issue, let's see if the community has ideas on how we can fix this. If we find a great root cause that's fixable we should do something.
Job added to Upwork: https://www.upwork.com/jobs/~0118d19fc456de16df
Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External
)
Triggered auto assignment to @NikkiWines (External
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
If we inspect this in the browser we can see the following elements and styles that render the message and edited label.
We have a div
with larger text and line-through
applied which displays the message. Inside it there's a span
with smaller text which displays edited label. As line-through is applied to the outer div
, the browser tries to render the line on the entire block. But because the font sizes are different, the line is not properly aligned. (AFAIK css spec does not have instructions on how to handle line-through with varying font sizes)
This behavior is a common issue and can be reproduced outside of Expensify. We can fix this by making the inner smaller text to be a separate block and apply line-through
to that separately.
Following is the component that renders the message and edited
label.
We can make the Text
compoent in L133 to have styles inline-block
and also include the line-through
. To do that we just need to update the styles as follows in L136
style={[...props.style, {display: 'inline-block'}]}
As pointed out by @sobitneupane the above solution alone doesn't address markdown comments. They have the same root cause described above but is rendered separately. So we need to add a fix there as well.
Markdown is rendered by following lines of code https://github.com/Expensify/App/blob/2d0ef06470d9f3a2c6b3692429fd7f3f78169336/src/pages/home/report/ReportActionItemFragment.js#L112-L124
This renders a custom <edited>
element defined in src/components/HTMLEngineProvider/HTMLRenderers/EditedRenderer.js
. We can add the same styles inline-block
and line-through
mentioned above to fix this. But this needs to be added under the condition of app being offline and has pending delete. Since there's no direct way to pass a prop into the EditedRenderer we can use an attribute to pass this information from the ReportActionItemFragment
This is how we can pass this attribute in ReportActionItemFragment L114
const editedTag = props.fragment.isEdited ? `<edited ${isPendingDelete ? 'deleted="true"' : ''}></edited>` : '';
EditedRenderer
needs to be updated as follows to read this attribute and apply the styles
// Component Code
const isPendingDelete = !!props.tnode.attributes.deleted;
// component jsx
style={isPendingDelete ? [styles.offlineFeedback.deleted, { display: 'inline-block'}] : undefined}
These changes will not have any impact on the web because span is already inline
and there's no visual difference between inline
and inline-block
. Also it will not have any impact on native apps as RN does not support inline-block
and so will be ignored. And yes, this works fine with edited single emoji.
Before
After
Setting the strike through using a pseudo element rather than browser native strike through was another option. This would give greater flexibility in positioning but I'd prefer to use browser features as much as possible.
π£ @pubudu-ranasinghe! π£
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:
Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Contributor details Your Expensify account email: pubudutr@gmail.com Upwork Profile Link: upwork.com/freelancers/~019eaf0e2e67ff1536
β Contributor details stored successfully. Thank you for contributing to Expensify!
Thanks for the proposal @pubudu-ranasinghe
Can you please add more explanation in "What is the root cause of that problem?" section. Please make use of permalink to link the places where the issue exists.
Can you please add more details in "What changes do you think we should make in order to solve the problem?" section as well. Please do include how you are going to achieve the suggested change. Also please let us know how will it impact native apps. Have you tested your solution with edited single emoji?
@pubudu-ranasinghe Thanks for the update.
Your proposal won't solve the issue for all the cases. It won't work in case of markdowns. Try your solution with inline codeblock (`abc`
).
@sobitneupane yes you are correct. This solution alone was not handling html comments. I've updated the original proposal to include a fix for this.
@sobitneupane thoughts?
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
Agreed! @pubudu-ranasinghe's proposal looks good π
π£ @pubudu-ranasinghe You have been assigned to this job by @NikkiWines! 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 π
Thanks, will post the PR within a day.
Sorry I was a bit busy for past couple of days. I've completed working on this and just need to test on all platforms and get screenshots. Will post the PR asap.
Where are we at with the PR review on this one @pubudu-ranasinghe @sobitneupane?
To be clear on this, is this issue caused by #17704? @sobitneupane
No, it is not. We were going to make use of changes made by https://github.com/Expensify/App/pull/17704. But looks like it is reverted.
Waiting for https://github.com/Expensify/App/pull/18523 PR to merge before proceeding with our PR as we will be using the change introduced in former PR.
@JmillsExpensify, @pubudu-ranasinghe, @trjExpensify, @NikkiWines, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!
@JmillsExpensify, @pubudu-ranasinghe, @trjExpensify, @NikkiWines, @sobitneupane Still overdue 6 days?! Let's take care of this!
The associated PR has now merged to main. We will be continuing with our PR.
Dope!
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.20-5 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2023-06-07. :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.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
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:
π @sobitneupane can you run through the checklist please so we can move on with processing payments? Thanks!
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:
I tried to go back to few commits to find the exact PR which introduced the issue. But could not point to the exact PR Looks like it was present from the very beginning when edited label was introduced in this PR.
Regression Test Proposal
Do we agree π or π
I tried to go back to few commits to find the exact PR which introduced the issue. But could not point to the exact PR Looks like it was present from the very beginning when edited label was introduced in https://github.com/Expensify/App/pull/2320.
Cool, fair enough. We comment there then! :)
As for the regression test, we have this one for editing a comment. We could pop it in the offline section. That said, I'd love to offer more direction than just "aligned properly", because who knows if the tester knows what's proper or not. Center aligned? What do you think?
Online
Offline
Bump on this @sobitneupane:
That said, I'd love to offer more direction than just "aligned properly", because who knows if the tester knows what's proper or not. Center aligned? What do you think?
Yup. Center aligned will make it clear.
As for the regression test, we have this one for editing a comment. We could pop it in the offline section.
I don't think we can add it in the section you mentioned above. We need to delete the message after editing in offline.
Ah yeah fair, okay.. I'll run it by applause. It could go in the delete offline tests perhaps. Issue created here.
Okay, let's proceed with the payments. I've sent offers for:
$1,000 to @sobitneupane for C+ $1,000 to @pubudu-ranasinghe for the fix $250 to @priya-zha for the bug report
@sobitneupane paid!
@priya-zha paid!
@pubudu-ranasinghe paid! Closing, thanks everyone.
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:
Expected Result: The horizontal line for the edited text should be at the center for the word 'edited' in a similar how android shows it
Actual Result:
The horizontal line for the word 'edited' is not at the center unlike android
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.97-2 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: Any additional supporting documentation
https://user-images.githubusercontent.com/43996225/230700746-c976069d-ccdd-49cc-8e4c-5cc01c52652b.mp4
Expensify/Expensify Issue URL: Issue reported by: @priya-zha Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680862318324249
View all open jobs on GitHub
Upwork Automation - Do Not Edit