Disable VBA inputs on native platforms when user reaches Personal information step #14611
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
59 tasks
6 tasks
melvin-bot
bot
requested review from
aimane-chnaif and
youssef-lr
and removed request for
a team
|
@aimane-chnaif @youssef-lr One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
luacmartins
changed the title
nkuoch
reviewed
Feb 1, 2023
| @@ -100,6 +100,7 @@ class BankAccountManualStep extends React.Component { | |||
| defaultValue={this.props.getDefaultStateForField('routingNumber', '')} | |||
| keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD} | |||
| disabled={shouldDisableInputs} | |||
Contributor
There was a problem hiding this comment.
I'd expect disabled to prevent editing .. why do we need both? Can't we make a global fix directly in TextInput?
Contributor
Author
There was a problem hiding this comment.
I'd expect disabled to prevent editing ..
AFAIK disabled is only a react-native-web prop. react-native itself uses editable and has no disabled prop available on TextInputs.
why do we need both?
disabled is not available in react-native so we can't use it for both browser and native platforms. Although editable is available in both react-native and react-native-web, it's equivalent in react-native-web is HTMLElement.readonly, instead of HTMLElement.disabled. That means that the input can't be changed, but we can still focus / blur which triggers style changes, etc.
Can't we make a global fix directly in TextInput?
We certainly can. I just kept it consistent with these other instances here and here, but we could change those as well. Would that be your preference?
Contributor
There was a problem hiding this comment.
I agree with @nkuoch here. What do you think about fixing it here? I just tested it and it seems to work well.
diff --git a/src/components/TextInput/BaseTextInput.js b/src/components/TextInput/BaseTextInput.js
index 87f02ba528..df118a524f 100644
--- a/src/components/TextInput/BaseTextInput.js
+++ b/src/components/TextInput/BaseTextInput.js
@@ -274,6 +274,7 @@ class BaseTextInput extends Component {
}}
// eslint-disable-next-line
{...inputProps}
+ editable={!this.props.disabled}
autoCorrect={this.props.secureTextEntry ? false : this.props.autoCorrect}
placeholder={placeholder}
placeholderTextColor={themeColors.placeholderText}
Contributor
Author
There was a problem hiding this comment.
Sure, I can fix it in BaseTextInput! We might need a bit more logic than that since there are instances where we want to disable it on mobile, but not necessarily on web e.g. here
Contributor
There was a problem hiding this comment.
I'm not familiar with platform specific code so far so I'm curious, wouldn't that code only run on Android?
Answering my own question and yes this is correct and it makes sense to check for editable the way you did.
Contributor
Author
There was a problem hiding this comment.
Correct! We have 3 DatePicker components (web, android and ios) each with its own logic - the code I linked above is from the android component and is run on android only.
Contributor
Author
|
Updated! |
youssef-lr
approved these changes
Feb 2, 2023
Contributor
youssef-lr
left a comment
youssef-lr
left a comment
There was a problem hiding this comment.
LGTM and tested well!
Contributor
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromeandroid.chrome.movMobile Web - SafarimWeb.safari.MP4Desktopdesktop.moviOSios.movAndroidandroid.mov |
Contributor
|
I will review shortly |
nkuoch
approved these changes
Feb 3, 2023
Contributor
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.mp4Desktopdesktop.moviOSios.mp4Androidandroid.movLooks good and tests well! |
Contributor
|
I also agree to fix this in low level (BaseTextInput) for platform consistency. iOS: App/src/components/Composer/index.ios.js Line 112 in 51f1a77 Android: App/src/components/Composer/index.android.js Line 107 in 51f1a77 Web: App/src/components/Composer/index.js Line 360 in 51f1a77 |
aimane-chnaif
approved these changes
Feb 3, 2023
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. |
Contributor
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
19 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 1.2.65-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.2.65-2 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Disabled
Company name,Tax ID,Routing numberandAccount numberinputs once the user has reached thePersonal informationstep.Additionally, from this issue, passing something else other than a Boolean to certain props, e.g. disabled, can cause crashes. This PR does not fix the mentioned issue, but it ensures that we pass the correct prop types.
cc @nkuoch
Fixed Issues
$ #14705
Tests
Settings > Workspace > Add bank account011401533and1111222233331111for routing and account numbers, respectivelySave & continuePersonal information, tap<to go back to theCompany stepCompany nameandTax IDfields anymore.<and thenContinue with setupRouting numberandAccount numberinputs are disabledOffline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov