-
Notifications
You must be signed in to change notification settings - Fork 2
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
Action bar - order and payment [last subscription cycle] #163
base: master
Are you sure you want to change the base?
Conversation
orderStatus: string | undefined | ||
orderDeliveryDate: string | undefined | ||
orderTrackingUrl: string | undefined | ||
bankSlipUrl: string | undefined |
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.
Maybe passing the full order to this guy is a better approach instead of multiple props.
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.
checking DetailsPage, the ActionBar
is called like this:
<ActionBar
status={subscription.status}
isSkipped={subscription.isSkipped}
address={subscription.shippingAddress}
payment={subscription.purchaseSettings.paymentMethod}
onUpdateAction={this.handleUpdateAction}
nextPurchaseDate={subscription.nextPurchaseDate}
orderStatus={subscription.lastExecution?.order?.status}
orderDeliveryDate={
subscription.lastExecution?.order?.status === 'invoiced'
? orderLogisticsInfo.length > 0 &&
orderLogisticsInfo[0].shippingEstimateDate
: undefined
}
orderTrackingUrl={
subscription.lastExecution?.order?.status === 'invoiced'
? orderPackages.length > 0 && orderPackages[0].trackingUrl
: undefined
}
bankSlipUrl={orderTransactions?.[0].payments?.[0]?.url}
/>
there are conditions in some parameters.. do you think it's better this way or passes the entire subscription object to handle it inside the action bar?
} else if (action === 'orderDispatched' && !!orderTrackingUrl) { | ||
window.open(orderTrackingUrl) |
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.
Displaying an anchor tag is a better approach instead of calling through javascript and you can do it on the ActionBar component itself instead of doing it on the parent component
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.
hmm I get it.. but based on the generic structure defined in this handleUpdateAction
method, that the effects are generated from the action
param, isn't it preferable to keep the navigations that way?
} else if (action === 'nextPurchase') { | ||
this.handleChangeEdit(true) |
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.
What does this do?
vtex-my-subscriptions-3/react/graphql/queries/subscriptionExecutions.gql
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
What does this PR do? *
New action bar behaviors for the last subscription's cycle.
How to test it? *
Related to / Depends on *