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 2023-06-23] [$1000] Allow adding newlines in description for tasks #19135

Closed
6 tasks
thienlnam opened this issue May 17, 2023 · 75 comments
Closed
6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@thienlnam
Copy link
Contributor

thienlnam commented May 17, 2023

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 a new task
  2. Add a description
  3. Try to add multiple lines

Expected Result:

Text input should expand to allow for multiple lines

Actual Result:

Does not allow for multiple lines

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?

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

Version Number:
Reproducible in staging?:
Reproducible in production?:
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
Expensify/Expensify Issue URL:
Issue reported by: Internal
Slack conversation: https://expensify.slack.com/archives/C04QEB4MJEQ/p1684309151082709

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01814d31ef4bb555e0
  • Upwork Job ID: 1658903725708025856
  • Last Price Increase: 2023-05-17
@thienlnam thienlnam added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 17, 2023

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

@melvin-bot melvin-bot bot changed the title Allow adding newlines in description for tasks [$1000] Allow adding newlines in description for tasks May 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 17, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01814d31ef4bb555e0

@melvin-bot
Copy link

melvin-bot bot commented May 17, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 17, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 17, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 17, 2023

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

@akinwale
Copy link
Contributor

akinwale commented May 17, 2023

Proposal

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

Task description text input should expand to allow for multiple lines

What is the root cause of that problem?

Feature request. The TextInput does not have the multiline nor autoGrowHeight props set.

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

Add the multiline and autoGrowHeight props to the following text inputs:

  1. TextInput with the taskDescription inputID in the NewTaskDetailsPage component.
  2. TextInput with the taskDescription inputID in the NewTaskDescriptionPage component.
  3. Updated after reading the discussion thread on Slack: TextInput with the description inputID in the TaskDescriptionPage component.

PS. Why do we have to see 2 different task description pages?

What alternative solutions did you explore? (Optional)

None.

@dhairyasenjaliya
Copy link
Contributor

dhairyasenjaliya commented May 17, 2023

Proposal

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

  • Allow adding newlines in description for tasks 

What is the root cause of that problem?

  • This is new feature task currently we don’t have functionality to assign task to our self

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

  • We need to add multiline on <Textinput> with number of line in all description occurrence
    we need to add on both page.s NewTaskDetailsPage.js and NewTaskDescriptionPage.js

  • Edit Proposal: We still have one page left to add multiLine which is in TaskDescriptionPage.js at the time of editing this proposal non of them added this page + containerStyles={[styles.mt5, styles.closeAccountMessageInput]} same as closeAccount

  • As per discussed we need to add autoGrowHeight props to above three pages as well textAlignVertical we can if needed

@dhairyasenjaliya
Copy link
Contributor

Proposal

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

  • Allow adding newlines in description for tasks 

What is the root cause of that problem?

  • This is new feature task currently we don’t have functionality to assign task to our self

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

  • We need to add multiline on <Textinput> with number of line in all description occurrence
    we need to add on both page.s NewTaskDetailsPage.js and NewTaskDescriptionPage.js

@dhairyasenjaliya
Copy link
Contributor

@akinwale don't edit the proposal after some other proposal you clearly edit did after my prospal which is not good practice

cc @sobitneupane please take this occurrence as well since this is a small issue they should have mentioned completely in first place

Screenshot 2023-05-18 at 12 12 04 AM

@akinwale
Copy link
Contributor

akinwale commented May 17, 2023

@akinwale don't edit the proposal after some other proposal you clearly edit did after my prospal which is not good practice

cc @sobitneupane please take this occurrence as well since this is a small issue they should have mentioned completely in first place

Screenshot 2023-05-18 at 12 12 04 AM

I posted my proposal and then ran tests before I made my edit. It only happens to be coincidence that your proposal came after. Am I not allowed to change my proposal because we arrive at the same conclusion?

@dhairyasenjaliya
Copy link
Contributor

dhairyasenjaliya commented May 17, 2023

@akinwale is it fair to other contributor? I believe post all changes on propsal sure minor changes can be fine

@akinwale
Copy link
Contributor

akinwale commented May 17, 2023

@akinwale is it fair to other contributor?

It is ultimately up to the C+ / internal engineer to decide. I discovered that I missed a page from my first post when I was testing, and then I updated my proposal. I was rushing to post my initial proposal as well, which is why I missed it in the first place.

@dukenv0307
Copy link
Contributor

dukenv0307 commented May 17, 2023

Proposal

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

Allow adding newlines in description for tasks

What is the root cause of that problem?

New feature

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

In description input in both NewTaskDetailsPage and NewTaskDescriptionPage Component

<TextInput
defaultValue={props.task.description}
inputID="taskDescription"
label={props.translate('newTaskPage.description')}
ref={(el) => (inputRef.current = el)}
/>

AND
<View style={styles.mb5}>
<TextInput
inputID="taskDescription"
label={props.translate('newTaskPage.descriptionOptional')}

and
<TextInput
inputID="description"
name="description"
label={props.translate('newTaskPage.description')}
defaultValue={(props.task.report && props.task.report.description) || ''}
ref={(el) => (inputRef.current = el)}
/>

We should add

 autoGrowHeight
 textAlignVertical="top"

Like in WorkspaceInviteMessagePage

<TextInput
inputID="welcomeMessage"
label={this.props.translate('workspace.inviteMessage.personalMessagePrompt')}
autoCompleteType="off"
autoCorrect={false}
autoGrowHeight
textAlignVertical="top"
containerStyles={[styles.workspaceInviteWelcome]}
defaultValue={this.state.welcomeNote}
value={this.state.welcomeNote}
onChangeText={(text) => this.setState({welcomeNote: text})}
/>

What alternative solutions did you explore? (Optional)

@NicMendonca NicMendonca removed their assignment May 17, 2023
@sobitneupane
Copy link
Contributor

@youssef-lr @flaviadefaria @thienlnam We have two proposals one to use autogrow as in WorkspaceInviteMessagePage and another to use Text Input with fixed number of lines as in "Close Account Page". I don't think autogrow carries much value when we have only one or two inputs with enough space.

Do we want the textinput to autogrow or have fixed height?

@thienlnam
Copy link
Contributor Author

Good question, we should probably consolidate how we handle these - could you please bring it up in #expensify-open-source? I don't have a strong opinion but maybe someone else does

@sobitneupane
Copy link
Contributor

@flaviadefaria
Copy link
Contributor

I'm OoO for 8 days so re-adding the BUG label, but keeping myself assigned. I'll be back at work on May 30th.

@flaviadefaria flaviadefaria added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels May 19, 2023
@akinwale
Copy link
Contributor

Sorry folks, one other new issue, #21449, was reported as stemming from this issue/PR. When you click Enter to add a newline in the description, it doesn't automatically expand the description field height until you start typing. Should this have been addressed in the PR here?

No, this is a pre-existing issue with the multiline TextInput as outlined in this comment. It was ultimately agreed that it would be treated (along with the other behaviour) in a separate issue as may be required.

@flaviadefaria
Copy link
Contributor

Ok as soon as @sobitneupane fills this out I can proceed with the payment.

@sobitneupane
Copy link
Contributor

@flaviadefaria There was a regression. It was handled by this PR. I think we should hold the payment for now.

@sakluger
Copy link
Contributor

@akinwale thanks for clarifying. Did you report those bugs in Slack? I just want to make sure there's no duplicate issue already created.

@akinwale
Copy link
Contributor

akinwale commented Jun 29, 2023

@akinwale thanks for clarifying. Did you report those bugs in Slack? I just want to make sure there's no duplicate issue already created.

No, I did not get a chance to do so.

@melvin-bot melvin-bot bot added the Overdue label Jul 3, 2023
@flaviadefaria
Copy link
Contributor

Waiting for the issue with the regression to close.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 3, 2023
@flaviadefaria
Copy link
Contributor

@flaviadefaria There was #20836. It was handled by #21371. I think we should hold the payment for now.

@sobitneupane it looks like the other GH is live. What should we do here? Are we ready for payment now? If so can oan you fill out the details here? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Jul 5, 2023
@akinwale
Copy link
Contributor

akinwale commented Jul 6, 2023

@flaviadefaria There was #20836. It was handled by #21371. I think we should hold the payment for now.

@sobitneupane it looks like the other GH is live. What should we do here? Are we ready for payment now? If so can oan you fill out the details here? Thanks!

@flaviadefaria @sobitneupane The PR addressing the regression has been in production for 7 days now, so I believe this is now eligible for payment, based on the guidelines.

Thanks.

@akinwale
Copy link
Contributor

@flaviadefaria @sobitneupane Bumping for an update. Would like to close this out. Thanks.

@sobitneupane
Copy link
Contributor

sobitneupane commented Jul 10, 2023

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 don't think BugZero Checklist applies here as it is a new feature not a bug.

@sobitneupane
Copy link
Contributor

Requested money on expensify.

@flaviadefaria
Copy link
Contributor

So just to confirm payments should be:
@sobitneupane $1000 - 50% from regression = $500
@akinwale $1000 - 50% from regression = $500

Is this correct?

@akinwale
Copy link
Contributor

So just to confirm payments should be: @sobitneupane $1000 - 50% from regression = $500 @akinwale $1000 - 50% from regression = $500

Is this correct?

@flaviadefaria Payment would be just $1000 for me. No speed bonus of $500 due since there was a regression.

@sobitneupane
Copy link
Contributor

Is this correct?

Yes, I think that's correct. I have requested $500 on Expensify app.

@akinwale
Copy link
Contributor

@flaviadefaria Is there anything else I need to do here? I haven't received an Upwork offer yet. Thanks.

@flaviadefaria
Copy link
Contributor

flaviadefaria commented Jul 13, 2023

@akinwale as you can see here this didn't meet the 3 days requirement to qualify for the bonus so the regression is deducted from the $1000, which leaves the payment at $500. I'll send you a contract for $500 now.

@flaviadefaria
Copy link
Contributor

Offer sent to you @akinwale!

@akinwale
Copy link
Contributor

@akinwale as you can see here this didn't meet the 3 days requirement to qualify for the bonus so the regression is deducted from the $1000, which leaves the payment at $500. I'll send you a contract for $500 now.

@flaviadefaria The contributing guidelines do not mention a deduction for regressions for contributors. The only thing mentioned is that a regression makes the issue ineligible for the bonus. The deduction only applies to C+.

Additionally, the reason this issue went beyond the 3-day requirement was due to review delays.

@akinwale
Copy link
Contributor

Offer sent to you @akinwale!

@flaviadefaria Accepted, thanks.

For reference, here's a previous issue I worked on where there was a regression. #18210

The final payments were $1000 / $500 for contributor / C+ respectively.

@flaviadefaria
Copy link
Contributor

It doesn't seem like the linked GH had a regression. Payment issued so closing this GH!

@akinwale
Copy link
Contributor

akinwale commented Jul 14, 2023

It doesn't seem like the linked GH had a regression. Payment issued so closing this GH!

I received $500, thanks.

However, as pointed out in this comment, there was a regression, and I subsequently refunded $500 from a payment of $1,500, and the C+ refunded $1,000.

Also, quoting from the contributing guide:

If the PR causes a regression at any point within the regression period (starting when the code is merged and ending 7 days after being deployed to production), contributors are not eligible for the 50% bonus.

There's nothing in the document about deducting 50% from the regular payment for a regression, as that only applies to C+. It only states that the contributor would be ineligible for the bonus.

Considering the above, there is still a balance of $500 due. Thanks.

@flaviadefaria
Copy link
Contributor

@akinwale you are right, sorry about that! I just sent you another offer for the remaining $500.

@akinwale
Copy link
Contributor

@akinwale you are right, sorry about that! I just sent you another offer for the remaining $500.

I've got it! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests