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

Add Keyboard disable and autogrow functionality to TextInput component #7318

Merged
merged 19 commits into from
Feb 10, 2022

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jan 19, 2022

Details

  • 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.

Fixed Issues

$ #6584

Tests | QA Steps

  1. Verify that the Input design is not changed for the profile page.
  2. Verify the same for a couple of other pages.
  3. Verify the same for Multiline TextInput such as on Workspace invite member page.
  4. Verify that you are able to do the following on IOUAmount page:
    a. Focus on input and cursor is shown.
    b. Height and font styles are not changed.
    c. Input is functioning correctly as before.
    d. Keyboard does not show up on focusing on the amount input.
  5. Input auto grow and shrink as usual.
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Desktop

image

Mobile Web

image

iOS

image

Android

image

@parasharrajat parasharrajat requested a review from a team as a code owner January 19, 2022 10:17
@MelvinBot MelvinBot requested review from puneetlath and removed request for a team January 19, 2022 10:17
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good to me and tests well.
Just some style changes.

src/components/TextInput/index.js Show resolved Hide resolved
src/components/TextInput/index.js Outdated Show resolved Hide resolved
src/components/TextInput/BaseTextInput.js Show resolved Hide resolved
src/components/TextInput/BaseTextInput.js Show resolved Hide resolved
src/components/TextInput/BaseTextInput.js Show resolved Hide resolved
src/components/TextInput/baseTextInputPropTypes.js Outdated Show resolved Hide resolved
src/components/TextInput/index.native.js Outdated Show resolved Hide resolved
src/pages/iou/steps/IOUAmountPage.js Outdated Show resolved Hide resolved
@rushatgabhane
Copy link
Member

LGTM! 🎉
All yours @puneetlath

@parasharrajat
Copy link
Member Author

@puneetlath Any thoughts?

@parasharrajat
Copy link
Member Author

@puneetlath Waiting for your review.

@puneetlath
Copy link
Contributor

Sorry for the delay. It looks good to me! Looks like you have one merge conflict.

And before I merge, I just want to confirm - @parasharrajat @rushatgabhane both of you have tested on all platforms?

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 27, 2022

I'll test once again after the merge conflicts have been resolved.

@parasharrajat
Copy link
Member Author

parasharrajat commented Jan 28, 2022

I will retest it and attach screenshots for all for visibility.

@parasharrajat
Copy link
Member Author

Updated screens and conflict resolved.

@puneetlath
Copy link
Contributor

Great. @rushatgabhane have you tested as well?

@rushatgabhane
Copy link
Member

Sorry for the delay. Placeholder color looks wrong for IOUAmountPage

@parasharrajat
image

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is werid. Validation for IOUAmount isn't working anymore. I can input letters.
main is working fine.

@parasharrajat can you merge main again to see if gets fixed?

src/styles/utilities/spacing.js Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

I will update this by tomorrow. Sorry for delay.

@parasharrajat
Copy link
Member Author

Here is what I am planning on to fix the issue.

    componentDidUpdate() {
        // activate or deactivate the label when value is changed programmatically from outside
        if (this.value === this.props.value) {
            return;
        }

        this.value = this.props.value;
        this.input.setNativeProps({text: this.value});

        if (this.props.value) {
            this.activateLabel();
        } else if (!this.state.isFocused) {
            this.deactivateLabel();
        }
    }

Explanation:

  1. TextInput implementation is incorrect. When the parent component modifies the values the TextInput is not updated properly. There are a couple of issues (unreported) for this.

    1. When the user typed acids.
    2. On the Consumer(parent component) we blocked the update of value as we are only accepting numbers. So value will be ''.
    3. Now on the TextInput the this.value === acids but this.props.value === '' and also the prevProps.value would be '' thus componentDidupdate will early return.
    4. but the text input will keep acids as this.value is still acids.

So I have moved the check to if (this.value === this.props.value) { from if (prevProps.value === this.props.value) {
Then as the component does not use state, I used this.input.setNativeProps({text: this.value}); to update the value. Which keeps it uncontrolled.
Last important part is to use the defaultValue instead of value on TextInput.

Pushing the changes for @rushatgabhane to test. Please test some other inputs in our app. I don't think anything is breaking but just to be sure.

@@ -46,13 +57,14 @@ class BaseTextInput extends Component {
this.input.focus();
}

componentDidUpdate(prevProps) {
componentDidUpdate() {
// activate or deactivate the label when value is changed programmatically from outside
Copy link
Member

@rushatgabhane rushatgabhane Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker: Comment should start with a captial letter 😅

rushatgabhane
rushatgabhane previously approved these changes Feb 8, 2022
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the detailed and simple explanation @parasharrajat 👏

@puneetlath Looks great, and tests well on all platforms.

@parasharrajat
Copy link
Member Author

parasharrajat commented Feb 8, 2022

@rushatgabhane Have you tested some other inputs in our app just to make sure it does not break anything? I have tested but another set of eyes is helpful.

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 8, 2022

@parasharrajat Yep, I've tested all types of inputs throughout the app twice. Nothing is broken

@puneetlath
Copy link
Contributor

Just one question from me @parasharrajat. Almost ready to merge!

@parasharrajat
Copy link
Member Author

parasharrajat commented Feb 10, 2022

Gentle bump @puneetlath. Let's merge this so that I can proceed to the next milestone.

@puneetlath puneetlath merged commit d8a3247 into Expensify:main Feb 10, 2022
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@parasharrajat
Copy link
Member Author

I think I incorrectly refactored a small recent change while merging main. It does not create any issue but I will correct that in my next PR.

@sig5 sig5 mentioned this pull request Feb 11, 2022
94 tasks
@luacmartins
Copy link
Contributor

I think this PR may have caused a regression when using TextInput as a form input. I now see the error below in the default Form story in Storybook:

Screen Shot 2022-02-15 at 1 42 44 PM

@parasharrajat would you mind taking a look?

@parasharrajat
Copy link
Member Author

Sure @luacmartins. I will and I also have a small change to be made. I will create the PR asap.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @puneetlath in version: 1.1.39-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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

Successfully merging this pull request may close these issues.

5 participants