-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 2023-09-07] [$1000] Line does not break in 'Description' of Request money #19872
Comments
Triggered auto assignment to @tjferriss ( |
Bug0 Triage Checklist (Main S/O)
|
@akinwale if I'm not mistaken you're working on a similar issue? |
ProposalPlease re-state the problem that we are trying to solve in this issue.Line does not break in 'Description' of Request money. What is the root cause of that problem?The description input is not multiline input. App/src/pages/iou/MoneyRequestDescriptionPage.js Lines 80 to 86 in 2157057
What changes do you think we should make in order to solve the problem?Make the description input multiline by add the following props multiline
textAlignVertical="top"
numberOfLines={6} // or 10 ResultScreen.Recording.2023-06-05.at.9.59.46.PM.movSupport multiline in
|
styles.pre, |
change title numberOfLines
to be numberOfLines={props.numberOfLines}
App/src/components/MenuItem.js
Line 193 in 9ed4a16
numberOfLines={1} |
What alternative solutions did you explore? (Optional)
we can instead use autoGrowHeight
prop with style maxHeight.
autoGrowHeight
containerStyles={[styles.moneyRequestDescriptionInput]}
// styles.moneyRequestDescriptionInput = {maxHeight: 115}
Result
Screen.Recording.2023-06-05.at.10.12.34.PM.mov
Alternative support multiline in MenuItem
prop types and default props.
/** Boolean whether to display the title in multiline */
multiline: PropTypes.bool,
// defaultProps
multiline: false,
use styles.pre in titleTextStyle
only if multiline false !props.multiline && styles.pre
App/src/components/MenuItem.js
Line 83 in 9ed4a16
styles.pre, |
change title numberOfLines
to be numberOfLines={props.multiline ? undefined : 1}
App/src/components/MenuItem.js
Line 193 in 9ed4a16
numberOfLines={1} |
@therealsujitk Yes, that is correct, but it's for a different area of the app, task descriptions. |
📣 @easeplan! 📣
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
I guess it the line didn't break because the individual didn't add whitespace between the text. HTML just took it as one. Add a proper description with whitespace, it should work. |
📣 @Oluwaniphise! 📣
|
Your Expensify account email: oduyaleenoch@gmail.com |
I don't think that is the case. You can add as many whitespaces as you want but the line is not breaking. |
@tjferriss I suppose this is possible, and I'm not opposed to this idea, but I'm not sure about all the details of how this combination would work re: the timeline, speed bonus, bounty (2x?) etc. I already have a PR open, and I can easily add the changes required for this in addition to making modifications to the MoneyRequestHeader component to support displaying multiline descriptions. You can follow the PR discussion for more details as I imagine the same set of challenges may also apply here. Additionally, there is a backend issue with saving task descriptions which contain newline characters. While I haven't tested yet, it may be possible that this could also happen with money requests. |
Does that mean it will not be considered as a bug for the tester? |
Allowing new lines involve change in preview as well. We should also discuss about change in preview before moving forward. Following are the two places (preview and report) which will be impacted by the change. In this issue, we are going to allow adding new lines to description for task assignment. We don't show description in task preview. So, we didn't have any issue in task preview. But for task report we decided to do nothing for now as task view is being redesigned as discussed here by @thienlnam. cc: @youssef-lr |
@akinwale sorry, I should have been more specific in my question. My question was whether we can close this issue in favor of the other one. It seems like the answer is no, we should treat these separately. |
Job added to Upwork: https://www.upwork.com/jobs/~010bfd86b76bb12ca6 |
Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new. |
What are we holding on now? Could we add it to the title if possible? |
Yes, @ahmedGaber93 can we move forward with your PR? |
@tjferriss yes PR #21664 is ready for review. |
Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:
On to the next one 🚀 |
^From the PR, we're waiting on QA |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.59-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-09-07. 🎊 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.
For reference, here are some details about the assignees on this issue:
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:
|
@tjferriss a kind reminder for payment |
BZ Checklist: Timeline analysis: |
@aimane-chnaif I don't understand. What do you mean it is a new feature? Does that mean there is no bonus for the reporter? |
@kavimuru @tjferriss It has been 4 days since the deadline for payment. |
@Habtamu-Asefa sorry for the delay. Can you please apply to the job on Upworks? Job posting: https://www.upwork.com/jobs/~011291928fb10896b6 |
@tjferriss Thanks for the response. I have sent a proposal to the link you gave. |
@tjferriss checking on the status of my payment |
@tjferriss Nine days since the payment deadline. I look forward to no further delay. |
Hired! @Habtamu-Asefa , can you please accept the job and reply here once you have? |
I applied the 50% bonus based on @aimane-chnaif 's timeline above Issue reporter: @Habtamu-Asefa paid $250 via Upwork @aimane-chnaif can you please complete the BZ checklist above? |
@mallenexpensify I also mentioned BZ checklist in #19872 (comment). So I think all items can be checked off. |
ah... I missed that. We def need new regression tests for new features, otherwise we'll never be able to check/test them going forward. Can you propose steps so I can create a TestRail GH @aimane-chnaif ? Thx |
Regression Test Proposal
|
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 line should break to a new line at some point
Actual Result:
There is no line break
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.20-3
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
screen-capture.5.webm
Recording.820.mp4
Expensify/Expensify Issue URL:
Issue reported by: @Habtamu-Asefa
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685338853319439
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: