[HOLD for payment 2022-04-01] Simplify/consolidate our various Text Input components

[puneetlath]

When doing #3133 and Expensify/eslint-config-expensify#38 I noticed that we have many different Text Input components. We have:

• ExpensiTextInput
• TextInputAutoWidth
• TextInputFocusable
• TextInputWithFocusStyles
• TextInputWithLabel

I don't think it's necessarily very obvious which of these should be used at a given time. Perhaps we can simplify these into one component and use props to differentiate things like whether they have a label or whether they are focusable.

cc @marcaaron since we were chatting about this in Expensify/eslint-config-expensify#38.

[MelvinBot]

Triggered auto assignment to @sketchydroide (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[puneetlath]

Will look into this more soon.

[puneetlath]

Omg! The list has grown!

• TextInput
• TextInputAutoWidthWithoutKeyboard
• TextInputFocusable
• TextInputWithFocusStyles.js
• TextInputWithLabel.js
• TextInputWithName
• TextInputWithPrefix

[parasharrajat]

Yeah, I agree we should do this. Maybe use composition of higher degree.

[puneetlath]

Any interest in taking it on?

[parasharrajat]

Yeah, I am interested. I will look into it more to prepare a plan and submit here. It sounds complicated but I like complex things.

[parasharrajat]

• TextInput
  I will merge everything in this component.

• TextInputAutoWidthWithoutKeyboard This can be easily merged.

  Purpose of component

  1. Hide the main keyboard. We can have this functionality behind a prop.
  2. Auto shrink and expand based on the entered text. This will be added as another prop. Although, currently it exists as a single component I feel this behavior is completely different from point 1.

• TextInputWithFocusStyles

  Purpose of component

  1. This is just used to change some styles when input is in a focused state and vice-versa.

  Currently, I feel its usage is unnecessary and this can be refactored to use TextInput. Otherwise, I will add used props to TextInput to reflect the same behavior.

• TextInputWithLabel
  Not Found

• TextInputWithName

  Purpose of component

  1. Support name attribute on Web brower.

  The same behavior can be easily moved to TextInput.

• TextInputFocusable

  Purpose of component

  1. Parse Mardown and Rich text formatting.
     I don't think we should merge this. This is dedicated to the composer. But I will rename it to something else to better reflect it.

• TextInputWithPrefix

  Purpose of component

  1. Add a prefix Text to the input field.
     Same this can be moved too behind a prop.

• Add Stories for all modes of the TextInput

[puneetlath]

This is great thank you @parasharrajat. All these changes sound great to me. Before we go forward, would you mind posting this in #expensify-open-source in slack so that we get a little bit more visibility on this? I think it will be a non-controversial change, but given that we didn't go through the normal process here I think it'd be good to get a little more visibility and also to have that as an example to point to in the future. Thanks!

[parasharrajat]

Here is the slack link for reference https://expensify.slack.com/archives/C01GTK53T8Q/p1642437182067200

[puneetlath]

Ok, thanks for posting that Rajat. It seems that there is general agreement on moving forward. My recommendation is that we break this up into several PRs in order to make it easier to manage. Let's do one PR for each component that is being merged into TextInput or updated. And then one PR for adding to storybook. We can do one Upwork job with each milestone being $250. Does that sound like a good/fair approach to you?

[parasharrajat]

Yeah, I am in favor of doing that. Thanks @puneetlath .

[MelvinBot]

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

[parasharrajat]

@michaelhaxhiu Could you please hold payment for me on this issue until I request/ping you. I would like to wait for a week. Please put this on weekly so that you don't get notifications from MelvinBot. Thanks.

[michaelhaxhiu]

Sure, I think we can just delay 1 week if you prefer? The daily label will get re-applied the day before the payout, so it'll remind us that way.

[rushatgabhane]

@michaelhaxhiu sorry to bother you, but if it isn't too much trouble, I'd prefer C+ payout to be anytime before March 31st.

(0 tax for me then 😁)

[michaelhaxhiu]

Totally - I intend to pay you today :)

[michaelhaxhiu]

List of PRs that @rushatgabhane should be compensated for as a C+ on this milestone, which I would really appreciate a buddy check on:

1. $125 - Add Keyboard disable and autogrow functionality to TextInput component #7318 - Rushat wasn't paid for C+ one this yet, and there was 1 regression
2. $125 - Refactor TextInputWithFocusStyles with TextInput  #7721 - there was 1 sneaky regression only.
3. $250 - [No QA] Merge TextInputWithName to our TextInput component #7722 - no regressions
4. $500 - Merge TextInputWithPrefix into TextInput #7937 - no regressions
5. $1000 - Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs #7984 - no regressions

Total payment for C+ reviews should be $2000 (even though there was 2 regressions)? Let me know if this sounds correct @rushatgabhane?

Would mind your eyes on this too @iwiznia or @NikkiWines (esp for #5 - I feel unclear on that more than anything because it's a huge PR. I just can't tell what exactly it's solving to allocate prices properly).

[parasharrajat]

I can help with the breakdown of work and milestones. @michaelhaxhiu

1. Add Keyboard disable and autogrow functionality to TextInput component #7318 - $250
   TextInputAutoWidthWithoutKeyboard
2. Refactor TextInputWithFocusStyles with TextInput  #7721 - $250
3. [No QA] Merge TextInputWithName to our TextInput component #7722 - $250
4. Merge TextInputWithPrefix into TextInput #7937 - $250 + bonus $250 added for [HOLD for payment 2022-03-22] Height of input box for entering room name is wrong and it does not highlight when focused - Reported by @parasharrajat #7308 (Merge TextInputWithPrefix into TextInput #7937 (comment))
5. Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs #7984 - $500
6. Add counter and hint to TextInput #7577 - $250 Not part of this contract but it was decided to pay me on the same contract [HOLD for payment 2022-03-02] Implement display character limit counter and hint on TextInput #7522 (comment)

Total payment for this issue - $1500 (excluding bonus at 4)

• 1 regression fix: crash when value is undefined #7782 for Add Keyboard disable and autogrow functionality to TextInput component #7318

Final payment for C+ - $1375

[michaelhaxhiu]

@rushatgabhane I'm afraid this won't be paid today, I don't feel confident in the final answer yet. Going to look at what @parasharrajat shared here and hopefully get input from another teammate to ensure this is accurate - thanks for that.

Will aim to pay you Monday

[rushatgabhane]

No worries, sounds good!

[rushatgabhane]

@parasharrajat for the 5th milestone, aren't these additional issues fixed in same PR? So the total is 1k including the issue itself. Please correct me if I'm missing something

MaxLength Counter is not incrementing automatically when a user types the value in the Story - #7984 (comment)

When the label is forced to be active. Placeholder text is not visible until you focus the input - #7984 (comment)

Autogrow does not work for unbound inputs- #7984 (comment)

[parasharrajat]

I was not paid other than the base milestone price to solve all of those issues. That PR refers to two milestones. So 500.

• TextInputFocusable

  Purpose of component

  1. Parse Mardown and Rich text formatting.
     I don't think we should merge this. This is dedicated to the composer. But I will rename it to something else to better reflect it.

• Add Stories for all modes of the TextInput

But this issue is not linked with this PR and thus I request another milestone on the current contract to fix this.

This never happened

[rushatgabhane]

I was not paid other than the base milestone price to solve all of those issues. That PR refers to two milestones. So 500.

Thanks, it makes sense now.

@parasharrajat @michaelhaxhiu so if we all agree that there are 3 new milestones, payment for #7984 should be :

$500 base + $750 for three new milestones = $1250

• MaxLength Counter is not incrementing automatically when a user types the value in the Story - Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs #7984 (comment)

• When the label is forced to be active. Placeholder text is not visible until you focus the input - Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs #7984 (comment)

• Autogrow does not work for unbound inputs- Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs #7984 (comment)

@michaelhaxhiu please let us know if any of the new milestones linked above are valid. We have consensus on everything else, thanks so much!

[michaelhaxhiu]

Thanks for pitching in (both of you!!), I realize this isn't a fun exercise and think the milestone got a little confusing - but that's ok. I just want to make sure you are both fairly compensated.

So do we agree that @rushatgabhane is owed $1250 for all C+ work captured in this final GH (#7984)? If that's wrong, let's do one last numbered list laying it out one by one.

[michaelhaxhiu]

Here's what I'm understanding based on the latest points:

1. $125 - Add Keyboard disable and autogrow functionality to TextInput component #7318 - Rushat wasn't paid for C+ one this yet, and there was 1 regression
2. $250 - Refactor TextInputWithFocusStyles with TextInput  #7721 - no regressions
3. $250 - [No QA] Merge TextInputWithName to our TextInput component #7722 - no regressions
4. $500 - Merge TextInputWithPrefix into TextInput #7937 - no regressions
5. $1250 - Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs #7984 - no regressions

So the total C+ payout is $2375? And that's because we added a few jobs to this contract that weren't initially a part of it?

[michaelhaxhiu]

My main question right now is about #2 - is #7782 a legit regression to consider? If so then C+ payout would be $2250.

[rushatgabhane]

Yes it's a legit regression. # 2 broke things like payments page which used to work before.

[parasharrajat]

Based on this I will also need to be paid more $750 for the three new milestones that Rushat is proposing.

So whatever is remaining on my contract + $750 for these new ones.

[michaelhaxhiu]

Yep, I agree with that. So we have the following payout for this milestone in order to close it out:

@parasharrajat - $2750
@rushatgabhane - $2250 (C+)

Can I get a 👍 from you both if this is good. Note to self - in future milestones, let's try to highlight payments as each GH deploys so it's easier to sum up in the end.

[parasharrajat]

Reminder: Please release the payment after tomorrow for me.

[michaelhaxhiu]

1. Updated your contract in upwork @parasharrajat.
   https://www.upwork.com/ab/c/8577561/contracts/29258288#milestones%2F20220131%2F20220301

2. Just paid you in upwork @rushatgabhane (https://www.upwork.com/jobs/~01495792016cebc27d)

[michaelhaxhiu]

All payments sent, closing.