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 2022-03-22] Height of input box for entering room name is wrong and it does not highlight when focused - Reported by @parasharrajat #7308

Closed
mvtglobally opened this issue Jan 19, 2022 · 34 comments
Assignees

Comments

@mvtglobally
Copy link

mvtglobally commented Jan 19, 2022

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. Open app
  2. Navigate to "+" > New Room
  3. Hit in the box or hover

Expected Result:

Input box should have blue border when focused.

Actual Result:

Height of input box for entering room name is wrong and it does not highlight when focused

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.30-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
image - 2022-01-18T194222 195

Upwork job link https://www.upwork.com/jobs/~01a4c8aadd3c3f0685
Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1642356868030500

View all open jobs on GitHub

@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

parasharrajat commented Jan 19, 2022

As I reported this, I would like to fix this issue in one of my PR for #6584. When I will refactor the TextInputWithPrefix, I will fix this too.

@neil-marcellini
Copy link
Contributor

Thanks I'll assign this one to you.

@parasharrajat
Copy link
Member

parasharrajat commented Jan 19, 2022

Thanks @neil-marcellini but maybe we can get this external first for reporting it.

@MelvinBot MelvinBot removed the Weekly KSv2 label Feb 14, 2022
@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

2 similar comments
@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@MelvinBot MelvinBot added the Monthly KSv2 label Feb 14, 2022
@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

6 similar comments
@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@MelvinBot
Copy link

This issue has not been updated in over 15 days. @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@parasharrajat
Copy link
Member

This needs to be exported for reporting bonus. I am happy to fix it under #6584. Randomly picking cc: @michaelhaxhiu as no one is assigned.

@michaelhaxhiu
Copy link
Contributor

@michaelhaxhiu
Copy link
Contributor

You will be paid for the fix in #6584, correct? So this will just be the $250 reporting bonus based on what I'm seeing!

@parasharrajat
Copy link
Member

So I am refactoring the same component as part of #6584 so I am happy to fix this on that and thus you are correct. Thanks.

@michaelhaxhiu
Copy link
Contributor

cool crystal clear 💎

@michaelhaxhiu
Copy link
Contributor

thanks for offering to do that also, @parasharrajat

@parasharrajat
Copy link
Member

@shawnborton I am planning to merge this TextinpuT(attached on the screenshot) In our usual TextInput. I have some questions about the design. I would be glad if you could help here. Thanks.

  1. What should happen to the label? Should I keep it above the input for when there is a prefix?
  2. What about the height of the input? Should it be small like this or normal as per TextInput?

@shawnborton
Copy link
Contributor

  • I think the height of this input should be just like other text inputs (52px)
  • I think in this case, we could try having a floating label that says "Room name" and remove the label above the input that we currently have. But I think it might look cleaner to not use a floating label. What do you think?

@parasharrajat
Copy link
Member

parasharrajat commented Feb 22, 2022

I am curious. How will it look with the floating label over the prefix and input area?

Are you suggesting that we just use the placeholder?

Here are some example..
image
image

@parasharrajat
Copy link
Member

Gentle bump @shawnborton

@shawnborton
Copy link
Contributor

Apologies, was OOO for the past few days.

One option is to just always keep the input as if it was in the "has value" state, but we actually move the input portion over to the right after a fixed #:
image

Thoughts on that?

@parasharrajat
Copy link
Member

Looks good to me.

@puneetlath
Copy link
Contributor

@michaelhaxhiu assigning you to make sure we pay Rajat for reporting this issue.

@michaelhaxhiu
Copy link
Contributor

#7937 (comment)
This comment suggests that the payout for rajat should go from $250 -> $500 for this GH right?

Additionally, he'll receive $250 via #6584, right?

@parasharrajat
Copy link
Member

So here is the summary for payout.

  1. I am fixing this as part of [HOLD for payment 2022-04-01] Simplify/consolidate our various Text Input components #6584. [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 (comment) => Payment to fix it.
  2. Here you will add reporting bonus
  3. @puneetlath mentioned adding a bonus for some additional work Merge TextInputWithPrefix into TextInput #7937 (comment).

I think I am fine without point 3. I say the change was small and quick so I don't want the bonus for that.

@michaelhaxhiu
Copy link
Contributor

I think we should pay you for point 3 and noticed puneet agreed too in his latest comment.

It sounds like the additional $250 should be added to the other GH #6584 (not this one)? Is that correct?

@parasharrajat
Copy link
Member

parasharrajat commented Mar 1, 2022

Yup that would be added to other issue not this one. Here only reporting bonus needs to be paid off.

@michaelhaxhiu
Copy link
Contributor

Thanks for confirming.

@michaelhaxhiu michaelhaxhiu added Weekly KSv2 and removed Monthly KSv2 labels Mar 11, 2022
@michaelhaxhiu
Copy link
Contributor

Waiting for this PR to go to production, and then I'll pay Rajat $250 for reporting this bug + close this GH.

@MelvinBot MelvinBot removed the Overdue label Mar 11, 2022
@michaelhaxhiu michaelhaxhiu changed the title Height of input box for entering room name is wrong and it does not highlight when focused - Reported by @parasharrajat [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 Mar 21, 2022
@michaelhaxhiu
Copy link
Contributor

Paid!

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

No branches or pull requests

7 participants