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] Android - Multiline TextInput height increases on app - reported by @thesahindia #7738

Closed
mvtglobally opened this issue Feb 14, 2022 · 30 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review

Comments

@mvtglobally
Copy link

mvtglobally commented Feb 14, 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 settings>security>close account
  3. Add multiline text in the box

Expected Result:

TextInput should not expand like on other platforms

Actual Result:

TextInput expands as user start typing

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Android

Version Number: 1.1.38-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2022-02-03.at.2.08.30.AM.mov

Expensify/Expensify Issue URL:
Issue reported by: @thesahindia
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643834819947769

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Feb 14, 2022
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Feb 14, 2022
@kakajann
Copy link
Contributor

Proposal

It's expected behavior for android TextInput with multiline. But if you like to not increase to height, you'll have to give it a fixed height.

This diff will fix the issue above.
file: src/pages/settings/Security/CloseAccountPage.js

<TextInput
    multiline
    numberOfLines={6}
    textAlignVertical="top"
    value={this.state.reasonForLeaving}
    onChangeText={reasonForLeaving => this.setState({reasonForLeaving})}
    label={this.props.translate('closeAccountPage.enterMessageHere')}
    containerStyles={[styles.mt5, styles.closeAccountMessageInput]}
+   inputStyle={[{
+       height: 140, // (lineHeight * numberOfLines) + labelHeight
+   }]}
/>

(of course, I will not use inline styling, it's just a demonstration)

There's a lot of TextInput with multiline prop. I can update them all if you accept my proposal.

Result

Screen.Recording.2022-02-14.at.1.36.33.PM.mov

@thesahindia
Copy link
Member

thesahindia commented Feb 14, 2022

Proposal

I reported this issue so proposing a solution -
We have two ways to fix this:-

  1. We can add inputStyles and can make the height 153px (not 140 or it will get smaller than what we currently have)
    or
  2. Instead of the minHeight in closeAccountMessageInput we can add height: 153

    App/src/styles/styles.js

    Lines 2445 to 2447 in 8431ed4

    closeAccountMessageInput: {
    minHeight: 140,
    },

    We are using closeAccountMessageInput here-
    containerStyles={[styles.mt5, styles.closeAccountMessageInput]}

@MelvinBot
Copy link

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

@flaviadefaria flaviadefaria removed their assignment Feb 14, 2022
@nkuoch
Copy link
Contributor

nkuoch commented Feb 15, 2022

cc @AndrewGable do we want to fix it?

@AndrewGable
Copy link
Contributor

Seems like we should make it match all the other platforms 👍

@nkuoch nkuoch added the External Added to denote the issue can be worked on by a contributor label Feb 16, 2022
@nkuoch nkuoch added the Weekly KSv2 label Feb 16, 2022
@MelvinBot
Copy link

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

@nkuoch nkuoch removed the Daily KSv2 label Feb 16, 2022
@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Feb 16, 2022
@nkuoch nkuoch added Weekly KSv2 and removed Daily KSv2 labels Feb 16, 2022
@nkuoch nkuoch removed their assignment Feb 16, 2022
@MitchExpensify
Copy link
Contributor

Upwork Job

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 17, 2022
@JalalMitali
Copy link

hello team, I have fixed this and I am ready to submit a pull request for review!

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 19, 2022

Hi @JalalMitali, I'd recommend you to take a look at https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#propose-a-solution-for-the-job

Feel free to submit a proposal, and ask any questions. Thanks!

@JalalMitali
Copy link

JalalMitali commented Feb 19, 2022

Proposal

what I did was I wrapped the multiline input text within a view and from that view I used the onlayout React Native call back function to get the height of the initialized multiline input while keeping the min height as it is.
I had to first add a state variable called firstRender to eliminate the need for resizing again ( height only ) through using an if statement and after checking ( getting the height ) we set it to false,
finally we use the height as maxHeight for the multiline text component.
**Result **
we did not have to set a fixed height for the component until it is rendered on the device which is good I guess :)
Screenshot_20220219-200625_New Expensify
Screenshot_20220219-200640_New Expensify

@JalalMitali
Copy link

dear @rushatgabhane I just applied for the job on upwork and I am ready to submit a pull request when needed I am done with the code already I have no problem with that :)

@rushatgabhane
Copy link
Member

Pause at this step until someone from the Contributor-Plus team and / or someone from Expensify provides feedback on your proposal (do not create a pull request yet).
If your solution proposal is accepted, Expensify will hire you on Upwork and assign the GitHub issue to you.

@JalalMitali again, this document explains the whole process. https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#propose-a-solution-for-the-job

@JalalMitali
Copy link

@rushatgabhane, thank you so much I will pause here dear and sorry for any inconvenience, this is only because it s my first time contributing to your project :)

@rushatgabhane
Copy link
Member

Oh no, that's no problem. I really appreciate you asking questions. I'd recommend that you join Expensify's opensource slack channel. Details here. Cheers!

@JalalMitali
Copy link

@rushatgabhane thank you I did ask to join also I updated my proposal to show some screenshots :)

@koushal141
Copy link

Proposal from Upwork

Hi,
The textinput is increasing its height because you must have included a prop multiline={true} but have set it's height manually via stylesheet rather than using a prop. This is a quick fix imo. We just need to add another prop to the input field for a fixed height.

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 22, 2022

🎀️👀️🎀️ C+ reviewed

Instead of the minHeight in closeAccountMessageInput we can add height: 153

@deetergp I like @thesahindia's proposal.
It should keep the UI consistent between different device font sizes (changed via accessibility settings). Also, this issue isn't happening anywhere else.

@deetergp
Copy link
Contributor

deetergp commented Mar 1, 2022

I agree with @rushatgabhane, we should go with @thesahindia's solution. 👍

@MitchExpensify
Copy link
Contributor

Great! I've invited both @thesahindia and @rushatgabhane to the UpWork Job, please both make a proposal and I'll hire you! Thanks

@MitchExpensify
Copy link
Contributor

Hired @thesahindia & @rushatgabhane, thanks!

@deetergp
Copy link
Contributor

The PR for this has been merged 🎉

@MelvinBot MelvinBot removed the Overdue label Mar 14, 2022
@deetergp deetergp added the Reviewing Has a PR in review label Mar 14, 2022
@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 15, 2022
@botify botify changed the title Android - Multiline TextInput height increases on app - reported by @thesahindia [HOLD for payment 2022-03-22] Android - Multiline TextInput height increases on app - reported by @thesahindia Mar 15, 2022
@botify
Copy link

botify commented Mar 15, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.42-6 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 2022-03-22. 🎊

@botify botify removed the Weekly KSv2 label Mar 22, 2022
@MelvinBot MelvinBot added the Daily KSv2 label Mar 22, 2022
@MelvinBot
Copy link

@deetergp, @rushatgabhane, @MitchExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@MitchExpensify
Copy link
Contributor

Payments made, contracts ended, closing!

@thesahindia
Copy link
Member

thesahindia commented Mar 30, 2022

@MitchExpensify, reporting bonus is pending on this. (it was fixed and reported by me)

@MitchExpensify
Copy link
Contributor

Paid @thesahindia! Thanks for the catch

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 Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests