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

Show a loading UI when accepting a dispute is in progress #7476

Merged
merged 14 commits into from
Oct 25, 2023
Merged

Show a loading UI when accepting a dispute is in progress #7476

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
aab1471
Disable accept dispute modal close when request in progress
Jinksi Oct 13, 2023
1d68e48
Disable accept dispute modal buttons when request in progress
Jinksi Oct 13, 2023
a141018
Show "Accept dispute" button loading animation when in progress
Jinksi Oct 13, 2023
28ac2e9
Leave accept dispute modal open when clicking accept or issue inquiry…
Jinksi Oct 13, 2023
20173b5
Add changelog entry
Jinksi Oct 13, 2023
ab3be4f
Merge branch 'develop' into fix/7473-accept-dispute-loading-state-ui
Jinksi Oct 18, 2023
32c1f0e
Merge branch 'develop' into fix/7473-accept-dispute-loading-state-ui
Jinksi Oct 23, 2023
528c7ca
Fix accept dispute modal max-width
Jinksi Oct 23, 2023
af9ec15
Use `Accepting` button label when in progress
Jinksi Oct 23, 2023
e7dc291
Merge branch 'develop' into fix/7473-accept-dispute-loading-state-ui
Jinksi Oct 24, 2023
24223b3
Merge branch 'develop' into fix/7473-accept-dispute-loading-state-ui
Jinksi Oct 24, 2023
b60c8dc
Rename isLoading → isDisputeAcceptRequestPending
Jinksi Oct 24, 2023
27e7621
Rename onModalClose → handleModalClose
Jinksi Oct 25, 2023
3e8f9ca
Merge branch 'develop' into fix/7473-accept-dispute-loading-state-ui
Jinksi Oct 25, 2023
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
4 changes: 4 additions & 0 deletions changelog/fix-7473-accept-dispute-loading-state-ui
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fix

Show loading state when accepting a dispute from the transaction details screen.
24 changes: 18 additions & 6 deletions client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,14 @@ interface AcceptDisputeProps {
/**
* Disputes and Inquiries have different text for buttons and the modal.
* They also have different icons and tracks events. This function returns the correct props.
*
* @param dispute
*/
function getAcceptDisputeProps( dispute: Dispute ): AcceptDisputeProps {
function getAcceptDisputeProps( {
dispute,
isLoading,
}: {
dispute: Dispute;
isLoading: boolean;
} ): AcceptDisputeProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

getAcceptDisputeProps is pretty strange to read and reason about. I think this code would be a lot clearer if it just had a dedicated modal for each case (inquiry vs dispute). AcceptDisputeProps is a domain-specific interface for a semi-generic modal (e.g. modalLines); I don't see a benefit to that over using the React components we have (i.e. a powerful, readable, flexible modal API).

That said, I don't know if it's safe/worthwhile to refactor this for this change. I would be tempted 😁

Mentioning because this PR reads as very decoupled – can't really see what changed. Specifically, the button Submitting… label should be an implementation detail inside a modal component. I find it strange (and complex) that this is delegated to a function (that now needs a new param).

Copy link
Member Author

@Jinksi Jinksi Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I agree and I began a refactor of this code in #7339, but it became low-priority and out-of-scope due to time constraints.

I think a refactor should be handled in a separate PR, as it is a more significant quantity of work and is separate from this PR's goal.

if ( isInquiry( dispute ) ) {
return {
acceptButtonLabel: __( 'Issue refund', 'woocommerce-payments' ),
Expand Down Expand Up @@ -146,7 +150,9 @@ function getAcceptDisputeProps( dispute: Dispute ): AcceptDisputeProps {
),
},
],
modalButtonLabel: __( 'Accept dispute', 'woocommerce-payments' ),
modalButtonLabel: isLoading
Jinksi marked this conversation as resolved.
Show resolved Hide resolved
? __( 'Accepting…', 'woocommerce-payments' )
: __( 'Accept dispute', 'woocommerce-payments' ),
modalButtonTracksEvent: wcpayTracks.events.DISPUTE_ACCEPT_CLICK,
};
}
Expand All @@ -171,6 +177,9 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
} = useContext( WCPaySettingsContext );

const onModalClose = () => {
if ( isLoading ) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

More renaming suggestions 😁

onModalClose reads strange to me – what is its purpose? Currently the name is based on where it is bound (a kind of coupling?). With the addition of the early return, it's becoming abstract, e.g. onModalClose() { if isLoading don't close }.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed it to handleModalClose in 27e7621 – does this make more sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

A tiny bit!

setModalOpen( false );
};

Expand All @@ -188,7 +197,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
);
};

const disputeAcceptAction = getAcceptDisputeProps( dispute );
const disputeAcceptAction = getAcceptDisputeProps( { dispute, isLoading } );

const challengeButtonDefaultText = isInquiry( dispute )
? __( 'Submit evidence', 'woocommerce-payments' )
Expand Down Expand Up @@ -326,6 +335,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
>
<Button
variant="tertiary"
disabled={ isLoading }
onClick={ onModalClose }
>
{ __(
Expand All @@ -335,6 +345,8 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
</Button>
<Button
variant="primary"
isBusy={ isLoading }
disabled={ isLoading }
Jinksi marked this conversation as resolved.
Show resolved Hide resolved
data-testid="accept-dispute-button"
onClick={ () => {
wcpayTracks.recordEvent(
Expand All @@ -346,7 +358,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
'transaction_details',
}
);
setModalOpen( false );
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why we didn't use onModalClose before.

Copy link
Member Author

@Jinksi Jinksi Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Same! I don't have an answer.


/**
* Handle the primary modal action.
* If it's an inquiry, redirect to the order page; otherwise, continue with the default dispute acceptance.
Expand Down
4 changes: 3 additions & 1 deletion client/payment-details/dispute-details/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@
}
}
.transaction-details-dispute-accept-modal {
max-width: 600px;
&.components-modal__frame {
max-width: 600px;
}

.components-modal__content {
padding-top: $grid-unit-30;
Expand Down
Loading