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.
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 VBA flow Part 1 #3459
Add VBA flow Part 1 #3459
Changes from 25 commits
ecb9990
c128cfe
5db6177
176b380
225ef6f
35e91bf
a84e2fd
78ebe95
8ae125e
ff91605
84cfe10
d9ed59a
6fbbf5f
5cdaec2
d5d1d99
f985e1a
5cfa72d
857ac9a
7d4a9c8
a098b66
7e96346
ac39938
45b86d0
8a19a67
d04ed46
108674f
6b3d5f5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure about this, but it looks like there's a button down at the bottom of this file that calls this function
onPress
, and hasisLoading={this.state.isCreatingAccount}
. I'm not entirely sure how this screen gets hidden, but I assume that it happens as a result ofonSubmit
. So I guess I'm wondering if we should switch this line and the previous one so that we set the loading state first, then perform theonSubmit
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's mostly just to show the loading spinner. Probably can be removed or improved in the future. This flow is a little half baked at the moment. I think it will need to be adjusted based on where this all goes next so I'm not super worried about it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue here to follow up on this eventually https://github.com/Expensify/Expensify/issues/166782
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, Should be moved to separate lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we enforce this so if eslint doesn't mind I don't mind 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: Why are we making
props.label
optional? Seems strange to make a label optional on a component calledTextInputWithLabel
. I also don't see you using theerrorText
prop anywhere where you're usingTextInputWithLabel
without thelabel
prop, so I'm wondering if this change is necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorta answered this in the other comment. But this component is likely going to change in the future when we apply some standardization to TextInputs across the app as it's kind of a mess. But we gotta keep rolling.