Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor to use an explicit prop for VBA loading state #9170

Merged
merged 4 commits into from
May 26, 2022
Merged

Refactor to use an explicit prop for VBA loading state #9170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
741c768
Refactor to use an explicit prop for VBA loading state
amyevans May 25, 2022
4a0bbeb
Remove unnecessary type cast
amyevans May 25, 2022
30e79ff
Refactor remaining instances of WorkspacePageWithSections
amyevans May 25, 2022
8423bb4
Disable currency picker while VBA state is loading
amyevans May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/pages/workspace/WorkspacePageWithSections.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,8 @@ class WorkspacePageWithSections extends React.Component {

render() {
const achState = lodashGet(this.props.reimbursementAccount, 'achData.state', '');
const hasVBA = this.props.reimbursementAccount.loading
? null
: achState === BankAccount.STATE.OPEN;
const isLoadingVBA = this.props.reimbursementAccount.loading;
const hasVBA = achState === BankAccount.STATE.OPEN;
const isUsingECard = lodashGet(this.props.user, 'isUsingExpensifyCard', false);
const policyID = lodashGet(this.props.route, 'params.policyID');
const policyName = lodashGet(this.props.policy, 'name');
Expand All @@ -112,7 +111,7 @@ class WorkspacePageWithSections extends React.Component {
>
<View style={[styles.w100, styles.flex1]}>

{this.props.children(hasVBA, policyID, isUsingECard)}
{this.props.children(isLoadingVBA, hasVBA, policyID, isUsingECard)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we changed up the order of arguments we must also check each usage of WorkspacePageWithSections. Spotted a few we missed:

2022-05-25_10-25-17

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Thank you for catching 🙏


</View>
</ScrollView>
Expand Down
4 changes: 2 additions & 2 deletions src/pages/workspace/WorkspaceSettingsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class WorkspaceSettingsPage extends React.Component {
</FixedFooter>
)}
>
{hasVBA => (
{(isLoadingVBA, hasVBA) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the current behavior is fine, but should we consider disabling the currency picker until we can confirm that the account does NOT have a VBA?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm kinda on the fence about it, but I suppose it looks better to go from disabled -> enabled than the reverse

<View style={[styles.pageWrapper, styles.flex1, styles.alignItemsStretch]}>
<AvatarWithImagePicker
isUploading={this.props.policy.isAvatarUploading}
Expand Down Expand Up @@ -176,7 +176,7 @@ class WorkspaceSettingsPage extends React.Component {
onInputChange={currency => this.setState({currency})}
items={this.getCurrencyItems()}
value={this.state.currency}
isDisabled={hasVBA}
isDisabled={isLoadingVBA || hasVBA}
/>
</View>
<Text style={[styles.textLabel, styles.colorMuted, styles.mt2]}>
Expand Down
7 changes: 4 additions & 3 deletions src/pages/workspace/bills/WorkspaceBillsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ const WorkspaceBillsPage = props => (
route={props.route}
guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_BILLS}
>
{(hasVBA, policyID) => (
{(isLoadingVBA, hasVBA, policyID) => (
<>
{!hasVBA ? (
{!isLoadingVBA && !hasVBA && (
Copy link
Contributor

@aldo-expensify aldo-expensify May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB and maybe out of scope: Just wondering if we should have a spinner when we hide the content when isLoadingVBA = true. I think the current behaviour just leaves the page white.

Copy link
Contributor Author

@amyevans amyevans May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah idk, curious for others' opinions here as well. I think when the full page is white, a spinner might be nice. But I think it'd look kind of odd on the Reimburse expenses screen where it'd be like 2 section blocks followed by a spinner. In that scenario, what would be nice is a gray skeleton (as proposed by @francoisl here). But considering we don't yet have those placeholder components, I'm not sure it makes sense to implement in the confines of this PR (...but also happy to if everyone is on board!).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I agree with Amy here and would wait for the skeleton components to be implemented.

<WorkspaceBillsNoVBAView policyID={policyID} />
) : (
)}
{!isLoadingVBA && hasVBA && (
<WorkspaceBillsVBAView policyID={policyID} />
)}
</>
Expand Down
8 changes: 4 additions & 4 deletions src/pages/workspace/card/WorkspaceCardPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,17 @@ const WorkspaceCardPage = props => (
route={props.route}
guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_CARD}
>
{(hasVBA, policyID, isUsingECard) => (
{(isLoadingVBA, hasVBA, policyID, isUsingECard) => (
<>
{!hasVBA && (
{!isLoadingVBA && !hasVBA && (
<WorkspaceCardNoVBAView policyID={policyID} />
)}

{hasVBA && !isUsingECard && (
{!isLoadingVBA && hasVBA && !isUsingECard && (
<WorkspaceCardVBANoECardView />
)}

{hasVBA && isUsingECard && (
{!isLoadingVBA && hasVBA && isUsingECard && (
<WorkspaceCardVBAWithECardView />
)}
</>
Expand Down
7 changes: 4 additions & 3 deletions src/pages/workspace/invoices/WorkspaceInvoicesPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ const WorkspaceInvoicesPage = props => (
route={props.route}
guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_INVOICES}
>
{(hasVBA, policyID) => (
{(isLoadingVBA, hasVBA, policyID) => (
<>
{!hasVBA ? (
{!isLoadingVBA && !hasVBA && (
<WorkspaceInvoicesNoVBAView policyID={policyID} />
) : (
)}
{!isLoadingVBA && hasVBA && (
<WorkspaceInvoicesVBAView policyID={policyID} />
)}
</>
Expand Down
4 changes: 2 additions & 2 deletions src/pages/workspace/reimburse/WorkspaceReimbursePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ const WorkspaceReimbursePage = props => (
route={props.route}
guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_REIMBURSE}
>
{(hasVBA, policyID) => (
<WorkspaceReimburseView policyID={policyID} hasVBA={hasVBA} />
{(isLoadingVBA, hasVBA, policyID) => (
<WorkspaceReimburseView policyID={policyID} isLoadingVBA={isLoadingVBA} hasVBA={hasVBA} />
)}
</WorkspacePageWithSections>
);
Expand Down
14 changes: 6 additions & 8 deletions src/pages/workspace/reimburse/WorkspaceReimburseView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@ const propTypes = {
/** The policy ID currently being configured */
policyID: PropTypes.string.isRequired,

/** Whether VBA data is loading */
isLoadingVBA: PropTypes.bool.isRequired,

/** Does the user have a VBA in their account? */
hasVBA: PropTypes.bool,
hasVBA: PropTypes.bool.isRequired,

/** Policy values needed in the component */
policy: PropTypes.shape({
Expand All @@ -48,10 +51,6 @@ const propTypes = {
...withLocalizePropTypes,
};

const defaultProps = {
hasVBA: null,
};

class WorkspaceReimburseView extends React.Component {
constructor(props) {
super(props);
Expand Down Expand Up @@ -195,7 +194,7 @@ class WorkspaceReimburseView extends React.Component {
</View>
</Section>

{this.props.hasVBA === false && (
{!this.props.isLoadingVBA && !this.props.hasVBA && (
<Section
title={this.props.translate('workspace.reimburse.unlockNextDayReimbursements')}
icon={Illustrations.JewelBoxGreen}
Expand All @@ -215,7 +214,7 @@ class WorkspaceReimburseView extends React.Component {
/>
</Section>
)}
{Boolean(this.props.hasVBA) && (
{!this.props.isLoadingVBA && this.props.hasVBA && (
<Section
title={this.props.translate('workspace.reimburse.fastReimbursementsHappyMembers')}
icon={Illustrations.BankUserGreen}
Expand All @@ -240,7 +239,6 @@ class WorkspaceReimburseView extends React.Component {
}

WorkspaceReimburseView.propTypes = propTypes;
WorkspaceReimburseView.defaultProps = defaultProps;
WorkspaceReimburseView.displayName = 'WorkspaceReimburseView';

export default compose(
Expand Down
7 changes: 4 additions & 3 deletions src/pages/workspace/travel/WorkspaceTravelPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ const WorkspaceTravelPage = props => (
route={props.route}
guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_TRAVEL}
>
{(hasVBA, policyID) => (
{(isLoadingVBA, hasVBA, policyID) => (
<>
{!hasVBA ? (
{!isLoadingVBA && !hasVBA && (
<WorkspaceTravelNoVBAView policyID={policyID} />
) : (
)}
{!isLoadingVBA && hasVBA && (
<WorkspaceTravelVBAView />
)}
</>
Expand Down