-
Notifications
You must be signed in to change notification settings - Fork 69
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
Transaction Details → Show resolution status footer for inquiries #7242
Transaction Details → Show resolution status footer for inquiries #7242
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: +91 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
@Jinksi started testing this and it feels pretty disconnected without the "you have an inquiry dispute you need to respond to" info box in transaction screen. Can you mention that in description and/or link to the relevant issue/PR for that aspect? |
As per the current design, this footer will actually appear in isolation – is the entire UI on the Transaction Details screen for The UI for Disputes that are |
I get that, and I understand how it works. My point is that as it currently stands the PR reviewer/tester needs to be aware that the critical middle part of the flow for inquiries is not implemented. And, if we shipped this (without the "you have an inquiry" bit), it would be ultra confusing for merchants. I think it's useful to mention that kind of detail in PR description. I think this is the issue that will cover that gap: |
Oh, I see now 😆 I thought you were referring to "the middle part of the current screen's UI", but I see you meant "the middle part of the flow". |
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.
Looks good! I tested all scenarios (and also "new inquiry" i.e. no response yet – no footer as expected) and this is working as designed. Checked mobile too.
See screenshots of my tests below.
Marking as approved although there is one thing that needs to be followed up:
- Inquiry closed footer is cramped on mobile (see screenshot below)
Also added some thoughts about reducing the data passed/exposed to the leaf components, and enforcing required data to avoid inappropriate conditional chaining.
Inquiry challenged & won
The closed footer is cramped on mobile:
Inquiry challenged – in review
Inquiry accepted - closed
client/payment-details/dispute-details/dispute-resolution-footer.tsx
Outdated
Show resolved
Hide resolved
client/payment-details/dispute-details/dispute-resolution-footer.tsx
Outdated
Show resolved
Hide resolved
@@ -273,6 +267,151 @@ const DisputeLostFooter: React.FC< { | |||
); | |||
}; | |||
|
|||
const InquiryUnderReviewFooter: React.FC< { | |||
dispute: Dispute; |
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.
Could potentially expand this out to the exact sub-fields needed (if we're not using all fields). Noted this because of dispute?.id
check later on – we should be able to enforce required data.
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.
Although the dispute?.id
check was resolved in comment #7242 (comment) as redundant, I've enforced required Dispute properties in 627c158 using Pick
:
dispute: Pick<
Dispute,
'id' | 'metadata' | 'status' | 'balance_transactions'
>
I could go further and explicitly destructure the individual props, e.g.:
- dispute={ dispute }
+ id={dispute.id} status={dispute.status}
But I think the Pick
statements sufficiently communicate the required 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.
Nice! TIL Pick 🙌
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.
Pick
also gets a mention in the WooPayments TS Guidelines. 🙌
Fixes #7191
Changes proposed in this Pull Request
This PR adds dispute information to the Transaction Details screen for closed or under review inquiries, with a link to the submitted evidence if available.
Inquiry: Under review
Inquiry: Closed (challenged)
Inquiry: Closed (accepted)
Mobile viewport
Testing instructions
Enable feature flag by running
update_option( '_wcpay_feature_dispute_on_transaction_page', '1' );
in WP Console or by modifying your database tablewp_options
directly.Inquiry: Under review
4000000000001976
at checkout.warning_needs_response
towarning_under_review
.Inquiry: Closed (challenged)
4000000000001976
at checkout.winning_evidence
in the “Additional details” field. This will change the dispute's status fromwarning_needs_response
towarning_closed
.Inquiry: Closed (accepted)
4000000000001976
at checkout.warning_needs_response
towarning_closed
.npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge