Revert remove hint #8232

rushatgabhane · 2022-03-19T00:03:54Z

Details

In PR #8145, hint feature was removed by mistake.
This PR adds back the hint feature.

Thanks to @parasharrajat for pointing this out here - #8044 (comment)

Fixed Issues

$ #8044

Tests

Test 1

npm run storybook
Go to TextInput -> Hint and error input
Verify that you see the hint - "Type Oops! to see the error" in grey color below the input.
Type Oops! in the input.
Verify that you see an error in red - "Oops! Looks like there's an error" below the input.
Erase Oops! from the input.
Verify that you see the hint "Oops! to see the error" again.

Test 2
Currently, we don't use hint prop of TextInput anywhere.
So the following steps are a workaround to test this PR.

Open /src/pages/settings/Payments/AddDebitCardPage.js
Add hint prop to any text input

<TextInput 
     hint="This is a hint"

Go to Settings -> Payments -> Add payment method -> Debit Card
Verify that a hint appears below the text input.
Click submit button.
Verify that the hint is replaced by an error message (if any)
Type something in the text input which has a hint.
Verify that the error is replaced by the hint.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Go to https://staging.new.expensify.com/docs/index.html?path=/story/components-textinput--hint-and-error-input
Verify that you see the hint - "Type Oops! to see the error" in grey color below the input.
Type Oops! in the input.
Verify that you see an error in red - "Oops! Looks like there's an error" below the input.
Erase Oops! from the input.
Verify that you see the hint "Oops! to see the error" again.

Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-03-19.at.3.02.05.AM.mov

Mobile Web

Screen.Recording.2022-03-19.at.2.30.08.AM.mov

Desktop

Screen.Recording.2022-03-19.at.2.17.58.AM.mov

iOS

Screen.Recording.2022-03-19.at.2.27.53.AM.mov

Android

screen-20220319-024324.mp4

rushatgabhane · 2022-03-19T00:12:43Z

@mountiny I'm sorry for approving the regression. I hope this fixes it.
Thank you so much for your time

parasharrajat · 2022-03-19T00:26:55Z

@rushatgabhane I think this should be QA'ed.

rushatgabhane · 2022-03-19T00:28:30Z

@parasharrajat hint isn't being used anywhere so I'm not sure if QA is possible.
Storybook isn't working either

src/components/TextInput/BaseTextInput.js

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

parasharrajat · 2022-03-19T00:38:30Z

Storybook isn't working either

Yeah. Hopefully, the fix is not far away. Let's add QA steps to test it in the storybook and we can wait for the storybook to be fixed. How does that sound? This way QA will come to know about this feature.

rushatgabhane · 2022-03-21T23:45:24Z

Sure, sounds good. I'll add the QA steps once the storybook bug is fixed

mountiny

@rushatgabhane Thanks for fixing this!

Can you please link where the Storybook bug is being worked on?

rushatgabhane · 2022-03-22T01:26:35Z

Storybook bug - #8179 (no PR for it yet)

neil-marcellini

This looks good to me once the storybook QA is added. It looks like the PR has been merged now.

mountiny

@rushatgabhane The Storybook fix has been merged #8265 🎉 feel free to update the steps 🙌 thanks!

rushatgabhane · 2022-03-22T23:23:00Z

wohoo! QA steps and tests have been updated.

mountiny

LGTM! @parasharrajat any thoughts here?

parasharrajat

Yup. As we are not showing the counter anymore, you should move the value prop from state to class property.

rushatgabhane · 2022-03-24T06:29:34Z

As we are not showing the counter anymore, you should move the value prop from state to class property.

@parasharrajat I pushed code which moves value to a class variable.

But are we sure that we wanna do this? The way we've designed autoGrow, text input won't grow if value isn't passed as a prop.

Because we're returning early in componentDidUpdate() when value isn't passed as a prop. So no re-renders will happen.
Please correct me if I'm wrong

App/src/components/TextInput/BaseTextInput.js

Lines 63 to 67 in 6e9d429

    
           componentDidUpdate() { 
        
               // Activate or deactivate the label when value is changed programmatically from outside 
        
               // Only update when value prop is provided 
        
               if (this.props.value === undefined || this.state.value === this.props.value) { 
        
                   return;

Screen.Recording.2022-03-24.at.9.11.40.AM.mov

rushatgabhane · 2022-03-24T06:39:31Z

#8232 (comment)

cc: @sig5 does moving value from state to a class variable help with your PR for modifying IOUAmount?
Is this why you're having trouble merging main into your branch?

parasharrajat · 2022-03-24T06:54:57Z

Oh, ok. May be directly update the width for autoGrow without depending on componentDidUpdate

rushatgabhane · 2022-03-24T10:54:49Z

src/components/TextInput/BaseTextInput.js

@@ -127,13 +124,13 @@ class BaseTextInput extends Component {
        if (this.props.onInputChange) {
            this.props.onInputChange(value);
        }
-        this.setState({value});
+        this.value = value;


May be directly update the width for autoGrow without depending on componentDidUpdate

We always have the correct value which is set by callback for onChangeText().

cont below..

rushatgabhane · 2022-03-24T11:04:27Z

src/components/TextInput/BaseTextInput.js

@@ -301,7 +302,7 @@ class BaseTextInput extends Component {
                        style={[...this.props.inputStyle, styles.hiddenElementOutsideOfWindow, styles.visibilityHidden]}
                        onLayout={e => this.setState({textInputWidth: e.nativeEvent.layout.width})}
                    >
-                        {this.state.value || this.props.placeholder}
+                        {this.value || this.props.placeholder}


But the updated value isn't reflected when calculating the width, because no re-renders happen on typing something in the input (which is a good thing).

This can only be solved by converting value to a state of the component.

parasharrajat · 2022-03-24T14:59:48Z

Ok. I see. Yup, I am fine with value in the state. You can revert those commits.

parasharrajat · 2022-03-24T15:01:04Z

src/components/TextInput/BaseTextInput.js

@@ -30,7 +29,6 @@ class BaseTextInput extends Component {
            passwordHidden: props.secureTextEntry,
            textInputWidth: 0,
            prefixWidth: 0,
-            value,


Add a comment here that says keep value in state for the AutoGrow feature to work.

sig5 · 2022-03-25T08:48:17Z

cc: @sig5 does moving value from state to a class variable help with your #7433?
Is this why you're having trouble merging main into your branch?

I checked again, it seems to work fine for me both ways.

rushatgabhane · 2022-03-25T14:02:46Z

I checked again, it seems to work fine for me both ways.

Thanks @sig5, we're safe to move value back to state now.

Commits reverted, and ready for review!

parasharrajat

Not tested. but LGTM.

…fy-App into revert-remove-hint

neil-marcellini

This looks good and the storybook tests work well.

mountiny

Thanks for doing this follow-up @rushatgabhane and thanks for the review @parasharrajat and @neil-marcellini Going to merge now 🎉

OSBotify · 2022-03-28T18:45:46Z

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

OSBotify · 2022-03-30T20:49:09Z

🚀 Deployed to staging by @mountiny in version: 1.1.47-0 🚀

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

OSBotify · 2022-04-05T17:31:15Z

🚀 Deployed to production by @roryabraham in version: 1.1.49-1 🚀

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

revert remove hint

13a45c0

rushatgabhane requested a review from a team as a code owner March 19, 2022 00:03

rushatgabhane marked this pull request as draft March 19, 2022 00:05

rushatgabhane marked this pull request as ready for review March 19, 2022 00:06

botify requested a review from mountiny March 19, 2022 00:06

parasharrajat suggested changes Mar 19, 2022

View reviewed changes

src/components/TextInput/BaseTextInput.js Outdated Show resolved Hide resolved

remove extra parentheses

1962e12

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

AndrewGable requested review from a team and removed request for a team March 21, 2022 22:23

melvin-bot bot requested review from neil-marcellini and removed request for a team March 21, 2022 22:24

mountiny reviewed Mar 22, 2022

View reviewed changes

neil-marcellini reviewed Mar 22, 2022

View reviewed changes

mountiny self-requested a review March 22, 2022 22:58

mountiny reviewed Mar 22, 2022

View reviewed changes

Merge branch 'Expensify:main' into revert-remove-hint

a410436

rushatgabhane changed the title ~~[No QA] Revert remove hint~~ Revert remove hint Mar 22, 2022

rushatgabhane requested a review from mountiny March 22, 2022 23:23

mountiny reviewed Mar 23, 2022

View reviewed changes

parasharrajat suggested changes Mar 23, 2022

View reviewed changes

Merge branch 'Expensify:main' into revert-remove-hint

90088fb

rushatgabhane commented Mar 24, 2022

View reviewed changes

parasharrajat reviewed Mar 24, 2022

View reviewed changes

add comment explaining why value is in state

970e9c2

rushatgabhane requested a review from mountiny March 25, 2022 14:02

Merge branch 'Expensify:main' into revert-remove-hint

65c1dc7

parasharrajat reviewed Mar 25, 2022

View reviewed changes

rushatgabhane added 2 commits March 25, 2022 17:16

cleanup - use _.isUndefined

fe37758

Merge branch 'revert-remove-hint' of github.com:rushatgabhane/Expensi…

16b4eac

…fy-App into revert-remove-hint

neil-marcellini approved these changes Mar 28, 2022

View reviewed changes

mountiny approved these changes Mar 28, 2022

View reviewed changes

mountiny merged commit e392f2c into Expensify:main Mar 28, 2022

rushatgabhane mentioned this pull request Mar 30, 2022

[HOLD for payment 2022-04-12] Remove max character limit from fields in business bank account / Wallet upgrade flows #8044

Closed

OSBotify mentioned this pull request Mar 30, 2022

Deploy Checklist: New Expensify 2022-03-30 #8378

Closed

47 tasks

kavimuru mentioned this pull request Mar 31, 2022

Payments - No hint appears in company or personal info pages when adding a debit card or bank account #8399

Closed

s77rt mentioned this pull request Jun 1, 2023

[HOLD for payment 2023-07-14] [$4000] Text selection options disappears abruptly #16696

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert remove hint #8232

Revert remove hint #8232

rushatgabhane commented Mar 19, 2022 •

edited

Loading

rushatgabhane commented Mar 19, 2022

parasharrajat commented Mar 19, 2022 •

edited

Loading

rushatgabhane commented Mar 19, 2022

parasharrajat commented Mar 19, 2022

rushatgabhane commented Mar 21, 2022

mountiny left a comment

rushatgabhane commented Mar 22, 2022

neil-marcellini left a comment

mountiny left a comment

rushatgabhane commented Mar 22, 2022 •

edited

Loading

mountiny left a comment

parasharrajat left a comment

rushatgabhane commented Mar 24, 2022 •

edited

Loading

rushatgabhane commented Mar 24, 2022 •

edited

Loading

parasharrajat commented Mar 24, 2022

rushatgabhane Mar 24, 2022

rushatgabhane Mar 24, 2022

parasharrajat commented Mar 24, 2022 •

edited

Loading

parasharrajat Mar 24, 2022

sig5 commented Mar 25, 2022

rushatgabhane commented Mar 25, 2022 •

edited

Loading

parasharrajat left a comment

neil-marcellini left a comment

mountiny left a comment

OSBotify commented Mar 28, 2022

OSBotify commented Mar 30, 2022

OSBotify commented Apr 5, 2022

Revert remove hint #8232

Revert remove hint #8232

Conversation

rushatgabhane commented Mar 19, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

rushatgabhane commented Mar 19, 2022

parasharrajat commented Mar 19, 2022 • edited Loading

rushatgabhane commented Mar 19, 2022

parasharrajat commented Mar 19, 2022

rushatgabhane commented Mar 21, 2022

mountiny left a comment

Choose a reason for hiding this comment

rushatgabhane commented Mar 22, 2022

neil-marcellini left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

rushatgabhane commented Mar 22, 2022 • edited Loading

mountiny left a comment

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

rushatgabhane commented Mar 24, 2022 • edited Loading

rushatgabhane commented Mar 24, 2022 • edited Loading

parasharrajat commented Mar 24, 2022

rushatgabhane Mar 24, 2022

Choose a reason for hiding this comment

rushatgabhane Mar 24, 2022

Choose a reason for hiding this comment

parasharrajat commented Mar 24, 2022 • edited Loading

parasharrajat Mar 24, 2022

Choose a reason for hiding this comment

sig5 commented Mar 25, 2022

rushatgabhane commented Mar 25, 2022 • edited Loading

parasharrajat left a comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Mar 28, 2022

OSBotify commented Mar 30, 2022

OSBotify commented Apr 5, 2022

rushatgabhane commented Mar 19, 2022 •

edited

Loading

parasharrajat commented Mar 19, 2022 •

edited

Loading

rushatgabhane commented Mar 22, 2022 •

edited

Loading

rushatgabhane commented Mar 24, 2022 •

edited

Loading

rushatgabhane commented Mar 24, 2022 •

edited

Loading

parasharrajat commented Mar 24, 2022 •

edited

Loading

rushatgabhane commented Mar 25, 2022 •

edited

Loading