Create a FormAlertWrapper that handles more complex components #9665
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
ctkochan22
changed the title
| /** Is the button in a loading state */ | ||
| isLoading: PropTypes.bool, | ||
| /** Submit function */ | ||
| onSubmit: PropTypes.func.isRequired, | ||
|
|
||
| ...withLocalizePropTypes, |
There was a problem hiding this comment.
I just re-arranged them alphabetically. And removed network
src/components/FormAlertWrapper.js
Outdated
| {props.isAlertVisible && ( | ||
| <View style={[styles.flexRow, styles.alignItemsCenter, styles.mb3]}> | ||
| <Icon src={Expensicons.Exclamation} fill={colors.red} /> | ||
| {getAlertPrompt()} |
There was a problem hiding this comment.
I think this is an anti-pattern. Anytime you feel the need to do this you should instead:
- Move the rendering to this place
- Create a separate component
Having another method that returns stuff to render is only complicating the render process.
| isAlertVisible: PropTypes.bool.isRequired, | ||
|
|
||
| /** Whether message is in html format */ | ||
| isMessageHtml: PropTypes.bool, |
There was a problem hiding this comment.
When would there be a message in HTML?
There was a problem hiding this comment.
It looks like it's only used in one spot this.props.reimbursementAccount.errorModalMessage and I really don't like it. I can live with it, but I don't like it. I don't like that there is a component with a prop that is only used for a single use case. For me, that means we should have a separate component like <FormHTMLAlertWrapper>.
I would also be extatic if we could make a backend change so that there is no HTML message coming from the server at all. I don't think the server should be responding with HTML.
There was a problem hiding this comment.
I'm pretty determined to remove this, but it may take some changes in Secure first. Lets address this after this PR
src/components/FormAlertWrapper.js
Outdated
|
|
||
| return ( | ||
| <View style={[styles.flexRow, styles.ml2, styles.flexWrap, styles.flex1]}> | ||
| {error} |
There was a problem hiding this comment.
Move all the rendering down here to be in one block. It's really difficult to look at this component and reason about the order things are rendered in.
src/components/FormAlertWrapper.js
Outdated
|
|
||
| return ( | ||
| <View style={[styles.mh5, styles.mb5, styles.flex1, styles.justifyContentEnd, ...props.containerStyles]}> | ||
| {props.isAlertVisible && ( |
There was a problem hiding this comment.
Why is isAlertVisible here instead of doing an early return at the top of render() and returning null? If the alert isn't supposed to be visible, why render the container with nothing in it?
|
Updated! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@ctkochan22 Does this need to be a PROD PR? |
|
Was this error not appearing before? @mvtglobally |
|
@ctkochan22 We were not explicitly testing negative scenarios, so I am not sure if this was appearing before or not. There was a PR in OldDot for this error, i can't find it, but i think @Justicea83 was working on it (if I remember right) |
|
I'll test it again. But the error looks good and is what the error should look like |
|
Just tested. The error is working as expected. the Button is a bit off style wise, which is not a blocker and will be addressed in a follow up PR |
|
🚀 Deployed to production by @roryabraham in version: 1.1.82-5 🚀
|
@Expensify/pullerbear
@tgolen @marcaaron @roryabraham
Create
FormAlertWrappercomponent, which allows you to wrap other components. We extract the offline/error-handling functionality fromFormAlertWithSubmitButtonand place it inFormAlertWrapper.For situations that requires more complex buttons, we can use
FormAlertWrapperby passing the components in as child props. For more basic buttons, we can still useFormAlertWithSubmitButton.Example: Button with Icons
This will allow us to work with buttons with icons.
Online
Offline
Example: ButtonWithDropdown
This will also work with
ButtonWithMenulike in SettlementButton.jsOnline:
Offline:
Fixed Issues
$ #9681
Tests
Uses of
FormAlertWithSubmitButtonAddDebitCardPage
40000000000099954000056655665556PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly 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)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly 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)QA Steps
Follow test steps. The test account number should throw out an error. Verify that it does. For the success case, please add one of your own debit cards.
Screenshots