Skip to content

[HOLD for payment 2023-05-23] [$1000] IOUPreview height should adjust based on content  #18770

Closed

Description

@Julesssss

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. Create an iou request to any user without adding a comment
  2. Settle the IOU
  3. Create an iou request to any user without adding a comment
  4. Settle the IOU

Expected Result:

  • The Card heights should be different

Actual Result:

  • The card heights are the same
  • The card without a description has a large gap where the comment would have shown

Native mobile
237498360-a96557f5-ee1f-4e71-be23-dd26fc659c7a (1)

Web
237498770-8c0574b2-9517-48a8-b763-46b3c29234f0

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.13-0
Issue reported by: @thesahindia here

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d6ca19ff51a2559a
  • Upwork Job ID: 1656655701807489024
  • Last Price Increase: 2023-05-11

Activity

  1. added
    BugSomething is broken. Auto assigns a BugZero manager.
    on May 11, 2023
  2. self-assigned this
    on May 11, 2023
  3. melvin-bot commented on May 11, 2023

    @melvin-bot

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

  4. melvin-bot commented on May 11, 2023

    @melvin-bot

    Bug0 Triage Checklist (Main S/O)

    • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
    • 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
    • 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.
    • 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.
    • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync
  5. huzaifa-99 commented on May 11, 2023

    @huzaifa-99
    Contributor

    Proposal

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

    We want the IOUPreview to adjust its height based on content, specifically if it has a comment/description

    What is the root cause of that problem?

    The <Text> element for comment/description here is rendering always, regardless if there is any comment/description

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

    We should conditionally render the <Text> component that displays comment/description here by first calculating description/value like this

    const requestDescription = Str.htmlDecode(lodashGet(props.action, 'originalMessage.comment', ''))

    and then render it like so

    {requestDescription && <Text>{requestDescription}</Text>}
    Result

    image

    What alternative solutions did you explore? (Optional)

    N/A

  6. allroundexperts commented on May 11, 2023

    @allroundexperts
    Contributor

    Proposal

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

    IOUPreview height does not adjust based on comment

    What is the root cause of that problem?

    The root cause is that we're always rendering the Text here.

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

    We should render the text only if a comment exists for IOU. This gets replaced with:

    // This can be saved in a variable as well
    {lodashGet(props.action, 'originalMessage.comment', '') ? <Text style={[styles.colorMuted]}>{Str.htmlDecode(lodashGet(props.action, 'originalMessage.comment', ''))}</Text> : null}
    

    Using && condition above will cause this regression. We should be using ternary instead.

    Although not strictly required, we can also convert this and this into ternary expressions as well.

    What alternative solutions did you explore? (Optional)

    None

  7. Julesssss commented on May 11, 2023

    @Julesssss
    ContributorAuthor

    thanks both. This is pretty straight forward so I'm going with the first passable proposal (#18770 (comment)). Also creating requestDescription on a separate line is a bit more readable.

  8. Julesssss commented on May 11, 2023

    @Julesssss
    ContributorAuthor

    @huzaifa-99 feel free to make a start on the PR, I'll assign you once the UpWork job has been created 👍

  9. Julesssss commented on May 11, 2023

    @Julesssss
    ContributorAuthor

    Actually I don't think anything will break if I do this now

  10. Julesssss commented on May 11, 2023

    @Julesssss
    ContributorAuthor

    Okay nevermind, that didn't trigger the automation, I'll do that again once Ted gets a chance to review.

  11. 35 remaining items

  12. huzaifa-99 commented on May 25, 2023

    @huzaifa-99
    Contributor

    Payment still pending guys 😄!

  13. mountiny commented on May 25, 2023

    @mountiny
    Contributor

    @trjExpensify was ooo but should be back today, sorry about that

  14. huzaifa-99 commented on May 25, 2023

    @huzaifa-99
    Contributor

    no worries @mountiny. Thank you!

  15. Julesssss commented on May 25, 2023

    @Julesssss
    ContributorAuthor

    The issue will remain open until paid, and Tom will be back soon :)

  16. trjExpensify commented on May 25, 2023

    @trjExpensify
    Contributor

    Yuuup, I got back today. 👍 Looking at this now, appreciate the patience.

  17. aimane-chnaif commented on May 25, 2023

    @aimane-chnaif
    Contributor

    No PR caused regression. This is more like polish than bug and found during PR review here.
    So no need regression test step.

  18. trjExpensify commented on May 25, 2023

    @trjExpensify
    Contributor

    Yup, this was when adding a new feature. Regression tests will be updated more broadly for the manual requests redesign.

  19. trjExpensify commented on May 25, 2023

    @trjExpensify
    Contributor

    Okay, so my assessment for payments is as follows:

    @aimane-chnaif - $1,500 for the C+ review & #urgency bonus
    @huzaifa-99 - $1,500 for the fix & #urgency bonus
    @thesahindia - $250 for reporting the bug in this PR

    Does that sound right to everyone?

  20. trjExpensify commented on May 25, 2023

    @trjExpensify
    Contributor

    Offers have been sent to all three of you in the meantime.

  21. huzaifa-99 commented on May 25, 2023

    @huzaifa-99
    Contributor

    Thank you @trjExpensify. Accepted!

  22. trjExpensify commented on May 25, 2023

    @trjExpensify
    Contributor

    Settled up with @huzaifa-99 & @aimane-chnaif. Awaiting @thesahindia and then I'll close this out.

    Job ref for ease.

  23. thesahindia commented on May 26, 2023

    @thesahindia
    Member

    Accepted, thanks!

  24. trjExpensify commented on May 26, 2023

    @trjExpensify
    Contributor

    Awesome, settled. Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

Awaiting PaymentAuto-added when associated PR is deployed to productionBugSomething is broken. Auto assigns a BugZero manager.DailyKSv2EngineeringExternalAdded to denote the issue can be worked on by a contributorImprovementItem broken or needs improvement.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions