[HOLD for payment 2023-05-16] [HOLD for payment 2023-05-09] [$1000] Placeholder padding inconsistent among different fields

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to any chat and open a IOU request modal
2. Enter any amount and go to the description page
3. Notice the Whats it for field padding.
4. Next, go to the home screen and click the search icon
5. Notice the padding for Name, email or phone number field.

Expected result:

Padding of the field to be consistent across pages.

Actual result:

Padding of the field is inconsistent.

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: 1.3.4
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

WhatsApp.Video.2023-04-23.at.12.16.47.PM.mp4

Recording.336.mp4

Expensify/Expensify Issue URL:
Issue reported by: @allroundexperts
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682234271392199

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0108c6b42413507adb
• Upwork Job ID: 1650563842550210560
• Last Price Increase: 2023-04-24

[MelvinBot]

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

[MelvinBot]

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

[allroundexperts]

Proposal

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

Placeholder padding is inconsistent among different pages

What is the root cause of that problem?

The root cause of the issue is that we're supplying placeholderText prop to the text input in the SearchPage as seen here where as we're using label for placeholder text in the MoneyRequestDescriptionPage as seen here.

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

The most simplest way to solve this is to replace this line with following:

label={this.props.translate('optionsSelector.nameEmailOrPhoneNumber')}

We can also deprecate the placeholderText and remove it from storybook as well.

I've also noticed that the spacing from top of Search page is different than other pages as well. Although unrelated to this ticket, but we can solve that as well by replacing styles.mv3 here with styles.mb3.

What alternative solutions did you explore? (Optional)

If we want to address the source of inconsistency, then we'll have to check the TextInput component itself. Inside it, we're applying a pv0 style if label is not present. This can be seen here. We can remove the !hasLabel condition from here to make the padding consistent with other pages. However, I feel like this might cause regressions. As such, I prefer the above solution more.

We can also act the other way around and make the TextInput in the MoneyRequestDescriptionPage use placeholderText instead of label. This can be seen here.

Note: Whatever solution we choose, must be applied to other components as well since the inconsistency appears there as well. If we fix this inside TextInput component itself, then all the inconsistencies will be catered for by default.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0108c6b42413507adb

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[parasharrajat]

@shawnborton Is this a bug in your opinion? This is how it has been on all pages.

[shawnborton]

Yeah, the search input was implemented wayyyy before we implemented the inputs that have the floating labels that move upwards when you focus or have a value.

That being said, we should make the search input match and use that same style.

[parasharrajat]

Thanks for the input @shawnborton. But we are using the same TextInput for search as well.

What happens is that because we don't have a floating label on the search, we don't need the padding top. Thus we remove the padding and vertically center the text, keeping the same height of 52px.

I remember that we have gone through this earlier while updating to floating design on TextInput. We decided to vertically center the placeholder and text for no label inputs.

Now, if we apply the padding same as normal textinput, this is how it will look.

Even if we apply the same padding for both top and bottom, the gap will remain same from bottom as this issue.

---

Please let me know what should be done here to keep the same styles.

[parasharrajat]

@allroundexperts Thanks for the proposal. I will review that once we have decided the expected change.

[shawnborton]

I think we should just use the floating label style for this input.

[parasharrajat]

Ok, so we should add a label to this input.

[shawnborton]

Yeah, just make the placeholder that's there now the label.

[parasharrajat]

@allroundexperts's main proposal looks good to me. Change the placeholder to label.

cc: @PauloGasparSv

🎀 👀 🎀 C+ reviewed

[PauloGasparSv]

Agreed so I'm assigning @allroundexperts here!

I've also noticed that the spacing from top of Search page is different than other pages as well. Although unrelated to this ticket, but we can solve that as well by replacing styles.mv3 here with styles.mb3.

Nice catch, let's do that too!

[MelvinBot]

📣 @allroundexperts You have been assigned to this job by @PauloGasparSv!
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 📖

[allroundexperts]

PR created #17991

[PauloGasparSv]

It seems we caused this regression and @allroundexperts created a fix P.R. for that!

I'll review it and CP it to staging here.

[MelvinBot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[PauloGasparSv]

Hey, I'm actually still checking if the deploy blocker #18154 was caused by changes made here.

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.8-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Use label instead of placeholder for option selector #17991
• fix: re-add placeholder text inside base option selector #18155

If no regressions arise, payment will be issued on 2023-05-09. 🎊

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.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.12-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: re-add placeholder text inside base option selector #18155

If no regressions arise, payment will be issued on 2023-05-16. 🎊

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.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[adeel0202]

Hi. I reported the regression here which was fixed in this PR, so I think that makes me eligible for the reporting bonus 😅

[isabelastisser]

Update, all contributors are hired in Upwork. Please accept it and I will process the payment, thanks!

@parasharrajat C+.
@allroundexperts issue reporter; accepted proposal.
@adeel0202 Regression issue reporter.

[isabelastisser]

Hey @parasharrajat , please complete the bugzero checklist above, thanks!

#17873 (comment)

[isabelastisser]

Update: all contributors were paid in Upwork.

@parasharrajat, please complete the checklist when you have a chance, thanks!

[parasharrajat]

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:

Not a bug but a change request.

[@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: NA

[@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not Required

[@parasharrajat] Determine if we should create a regression test for this bug. No

[@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. #17873 (comment)

[parasharrajat]

Regression Test Steps

1. Go to any chat and open an IOU request modal
2. Enter any amount and go to the description page
3. Notice the What's it for field. The field should have a normal TextInput style where the label floats up on focus.
4. Go to the Search page, New Chat page, and new group page one by one.
5. Observe that the search field should have a normal TextInput style where the label floats up on focus.
6. Repeat the same steps for Workspace Member Invite, Request money & Split money pages.

Do you agree 👍 or 👎 ?

[isabelastisser]

Based on the C+ recommendation, we are all set to close this issue now.

[@parasharrajat] Determine if we should create a regression test for this bug. No