-
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 steps to resolve for inquiries #7292
Transaction Details → Show steps to resolve for inquiries #7292
Conversation
To match Stripe dispute object API See docs https://stripe.com/docs/api/disputes/object
@@ -28,6 +28,42 @@ interface Props { | |||
daysRemaining: number; | |||
} | |||
|
|||
const DueByDate: React.FC< { |
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.
I moved this out to a standalone shared component in 5c8a008 since it is relatively complex logic and is identical for both cases.
@@ -76,7 +76,7 @@ export type DisputeStatus = | |||
export interface Dispute { | |||
status: DisputeStatus; | |||
id: string; | |||
evidence_details?: EvidenceDetails; | |||
evidence_details: EvidenceDetails; |
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.
This is a required field as per the Stripe dispute API docs.
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: +220 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
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.
This UI component <DisputeDueByDate />
is now shared by 3 components on this screen, so I've moved it to it's own component file.
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 and I didn't see any major issues in my testing.
I tested:
- Inquiry due today
- Inquiry due soon
- Dispute due soon
- Regular purchase
One minor blocker – there's an extra line under the last list item.
The other thing I noticed is that the coloured Due soon
Due now
Past due
text is rendered "inside" the sentence, which feels inconsistent to me.
I think that of that highlight as being stamped on to the screen to signal urgency, rather than "inserted" into the copy. So if it was me, I'd move that out, and then the two places on the screen would look more consistent (the one above is not part of a sentence).
However, with this nice new component rendering both values, and a sense of urgency, I only raise this as something to consider in future. @nikkivias FYI, curious for your thoughts
client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
Outdated
Show resolved
Hide resolved
dueByDate: ( | ||
<DisputeDueByDate | ||
dueBy={ dispute.evidence_details.due_by } | ||
/> |
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.
Love the way you've decoupled this into a self-contained component, and simplified props in the process.
), | ||
{ | ||
a: ( | ||
// eslint-disable-next-line jsx-a11y/anchor-has-content |
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.
😭
__( '(Past due)', 'woocommerce-payments' ) } | ||
</span> | ||
</span> | ||
<DisputeDueByDate dueBy={ dispute.evidence_details.due_by } /> |
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.
🔥 🚀 🏅
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.
Oh god sorry for the late reply on this I get lost in the Github threads 🥲 please forgive me. I agree with your comment @haszari it should appear more as a stamp instead of an insert in that last bullet point. However! I'm looking at the latest designs, the second [Due in X days] was removed, and the only one that should remain is the one in the dispute details. That's my fault, I didn't specifically call that out in the Figma file. Can we please remove, or do I need to create an issue? @Jinksi
On another note, I'm curious as you talk about the extra line at the bottom, why are the buttons not visible here?
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.
Can we please remove, or do I need to create an issue?
@nikkivias if you can make an issue to get us started we'd really appreciate it! We can handle this as maintenance, should be a relatively painless fix.
On another note, I'm curious as you talk about the extra line at the bottom, why are the buttons not visible here?
I suspect this is an old screenshot from when those specific action buttons were not implemented. Feel free to test and if you see any gaps chat in slack or just make an issue 🙌
Fixes #7245 (Inquiry steps to resolve)
Fixed #7255 (Show "Past due" in steps to resolve when dispute past due)
Changes proposed in this Pull Request
Add the 'Steps to resolve' section on the transaction details page for inquiries.
Notes:
Testing instructions
update_option( '_wcpay_feature_dispute_on_transaction_page', '1' );
in WP Console.4000000000001976
at checkout.Problem with your purchase from {store name} on {YYYY-MM-DD of the charge date}?
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