Skip to content

Fix broken max length in "SNN Last 4 digits" input #7551

Merged
luacmartins merged 2 commits into
mainfrom
aldo_fix-ssn4-max-length
Feb 3, 2022
Merged

Fix broken max length in "SNN Last 4 digits" input #7551
luacmartins merged 2 commits into
mainfrom
aldo_fix-ssn4-max-length

Conversation

@aldo-expensify

@aldo-expensify aldo-expensify commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

cc @luacmartins: this was a change related to the new form handling, can you check that I'm not removing/breaking something that was necessary

Another property

Details

Explanation here: #7548 (comment)

Remove maxLength prop from baseTextInputPropTypes
The prop is not directly used by this component

Fixed Issues

$ #7548

Tests / QA

SO on VBA info: https://stackoverflow.com/c/expensify/questions/342/525#525

  1. Launch the app
  2. Log in with expensiafail account
  3. Go to any Workspace - Connect Bank Account
  4. Tap Connect online with Plaid
  5. Select Wells Fargo Bank Account
  6. Put username: user_good, password: pass_good
  7. Tap Submit
  8. Tap on Plaid Saving - Save and continue
  9. Put all necessary information in Compony Information Page
  10. Tap Save and Continue
  11. In SSN box start typing as many number you are want
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

The prop is not directly used by this component
@aldo-expensify aldo-expensify requested a review from a team as a code owner February 3, 2022 16:32
@aldo-expensify aldo-expensify self-assigned this Feb 3, 2022
@MelvinBot MelvinBot removed the request for review from a team February 3, 2022 16:32
@github-actions

github-actions Bot commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@MelvinBot MelvinBot requested a review from cead22 February 3, 2022 16:32
@cead22

cead22 commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

Should this be tested on all platforms?

@aldo-expensify

Copy link
Copy Markdown
Contributor Author

The prop hint also got added there in the same PR:

/** Hint microcopy to be displayed under the input */
hint: PropTypes.string,

But I don't think it is being used anywhere. Should we remove it?

@aldo-expensify

aldo-expensify commented Feb 3, 2022

Copy link
Copy Markdown
Contributor Author

Should this be tested on all platforms?

I guess it should.

Do the checkboxes mean that it "has been tested"? or that is "has to be tested"? I was assuming the first

@cead22

cead22 commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

I'm going off of the slack convos pushing for tests on all platforms on all PRs.

Also, it looks like baseTextInputPropTypes is used by TextInput, BaseTextInpupt, and DatePicker (indirectly). Can you make sure to test that none of those inputs are breaking in your tests?

@cead22

cead22 commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

But I don't think it is being used anywhere. Should we remove it?

I think so

@aldo-expensify

Copy link
Copy Markdown
Contributor Author

Also, it looks like baseTextInputPropTypes is used by TextInput, BaseTextInpupt, and DatePicker (indirectly). Can you make sure to test that none of those inputs are breaking in your tests?

A note on this, the maxLength prop in the is used by the react-native component TextInput (same name as ours). I understand that our approach is not to put props in the definitions if they are just drilled down.

@aldo-expensify

Copy link
Copy Markdown
Contributor Author

I'll test on other platforms now

Comment thread src/components/TextInput/baseTextInputPropTypes.js
@aldo-expensify

Copy link
Copy Markdown
Contributor Author

Update: tested in all platforms, seems to be working fine.

@luacmartins

Copy link
Copy Markdown
Contributor

Merging since it's a revert of my changes.

@luacmartins luacmartins merged commit cb9f1b4 into main Feb 3, 2022
@luacmartins luacmartins deleted the aldo_fix-ssn4-max-length branch February 3, 2022 18:10
OSBotify pushed a commit that referenced this pull request Feb 3, 2022
Fix broken max length in "SNN Last 4 digits" input

(cherry picked from commit cb9f1b4)
@OSBotify

OSBotify commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by @luacmartins in version: 1.1.35-1 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify

OSBotify commented Feb 4, 2022

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @sketchydroide in version: 1.1.35-1 🚀

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.

4 participants