-
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
Add dispute details screen #418
Conversation
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.
@dechov great progress on that! 👍
I've tested with fraudulent and product not received disputes and confirm different content is shown for each of them. However, I tried to check if messaging matches the gdoc but not sure what's the correct version so leaving this check to the @LevinMedia . I've noticed headings in the message are a bit bigger than card title, not sure that's correct. @LevinMedia will
Some general notes:
- Noticed that one test snapshot is out of date, see
npm test
output for details. - Phpunit tests are failing as well but the reason seems to be unrelated to the current changes.
See other notes and comments below.
7e1b975
to
1d2f2f7
Compare
@vbelolapotkov Should have mentioned, the copy is from p1581023557475400-slack-wcpay (thread). Rebased on #414 branch to pick up the good work there, and tweaked a number of things in the details and challenge screens to have them match the designs better. |
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.
@dechov it looks good to me! 🎉
I'd only ask you to add a comment/todo to the disputes styling.
@@ -0,0 +1,54 @@ | |||
.wcpay-dispute-details, .wcpay-dispute-evidence { | |||
$horizontal-card-padding: 24px; |
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 feels a bit weird to me having special padding for disputes/evidence screens. IMO it either should be the same for deposits and transactions as well or should be achieved with some utility classes.
Anyway, I don't feel it's a good enough reason to hold the PR from being merged so I suggest adding a comment or TODO to address this in the future.
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.
👍 Done in e5953e2
e5953e2
to
48da12f
Compare
@dechov thanks for the keen eye on the type styles. If you could, please align them to the type scale described in the Figma file. The card headers use The card bodies use The bold text that separates paragraphs in the second card are also per our slack chat - hopefully type scale linked above will soon be included with gutenberg, until that time we can hard code it. If too much time passes though, we'll probably want to include the type scale as part of @woocommerce/components - as we're using it elsewhere in new designs across WooCommerce such as onboarding and other new screens in wc-admin. |
Evidence stylesheet was accidentally affecting other screens to good effect. This change restores the appearance and further tweaks the styling.
48da12f
to
e39de1f
Compare
@LevinMedia With your blessing, I've opened #427 to track the type style update. |
Fixes #350
Changes proposed in this Pull Request
Add a "Dispute Details" screen with a Challenge button linking to the evidence form:
…linked via a new info button in Disputes list rows:
The copy for the details page is derived from a google doc shared by @LevinMedia.
Also:
Treating container width and outsize vertical padding (being discussed in #414) as out of scope.
The dispute info section near the top is tracked in #379.
The "Accept Dispute" button can also be a different PR in my view (not currently tracked in its own issue but blocking #376 and #377).
Question: should the info button only appear if the dispute needs a response? Or for all rows, as now, except we'll need to disable things / make it clear it's not actionable? (cc @LevinMedia; related: #379 (comment))
Testing instructions