Payouts: Show bank reference key on payout details page #9886
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Significance: minor | ||
| Type: add | ||
|
|
||
| Show Bank reference key on top of the payout details page, whenever available. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| /** | ||
| * External dependencies | ||
| */ | ||
| import React, { useEffect } from 'react'; | ||
| import { dateI18n } from '@wordpress/date'; | ||
| import { __, sprintf } from '@wordpress/i18n'; | ||
| import moment from 'moment'; | ||
|
|
@@ -70,7 +70,7 @@ interface SummaryItemProps { | |
| label: string; | ||
| value: string | JSX.Element; | ||
| valueClass?: string | false; | ||
| detail?: string | JSX.Element; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -130,28 +130,82 @@ export const DepositOverview: React.FC< DepositOverviewProps > = ( { | |
| depositDateLabel = __( 'Withdrawal date', 'woocommerce-payments' ); | ||
| } | ||
|
|
||
| const DepositDateItem = () => { | ||
| useEffect( () => { | ||
| const copyButton = document.querySelector( | ||
| '.woopayments-copy-bank-reference-key' | ||
| ); | ||
|
|
||
| if ( copyButton ) { | ||
| copyButton.addEventListener( 'click', () => { | ||
| const bankReferenceKey = document.querySelector( | ||
| '.woopayments-payout-bank-reference-key' | ||
| )?.textContent; | ||
|
|
||
| if ( bankReferenceKey ) { | ||
| navigator.clipboard.writeText( bankReferenceKey ); | ||
|
|
||
| copyButton.classList.add( 'state--copied' ); | ||
| setTimeout( () => { | ||
| copyButton.classList.remove( 'state--copied' ); | ||
| }, 2000 ); | ||
| } | ||
| } ); | ||
|
There was a problem hiding this comment. Can we implement the copy button behaviour in a more React-idiomatic way, without using a side effect? There was a problem hiding this comment. I have changed this and used |
||
| } | ||
| }, [] ); | ||
|
|
||
| return ( | ||
| <SummaryItem | ||
| key="depositDate" | ||
| label={ | ||
| `${ depositDateLabel }: ` + | ||
| dateI18n( | ||
| 'M j, Y', | ||
| moment.utc( deposit.date ).toISOString(), | ||
| true // TODO Change call to gmdateI18n and remove this deprecated param once WP 5.4 support ends. | ||
| ) | ||
| } | ||
| value={ <DepositStatusIndicator deposit={ deposit } /> } | ||
| detail={ | ||
| <> | ||
| { deposit.bankAccount } | ||
| <br /> | ||
| Bank reference key:{ ' ' } | ||
| { deposit.bank_reference_key ? ( | ||
| <> | ||
| <span className="woopayments-payout-bank-reference-key"> | ||
| { deposit.bank_reference_key } | ||
| </span> | ||
| <button | ||
| type="button" | ||
| className="woopayments-copy-button-bank-reference-key" | ||
| aria-label={ __( | ||
| 'Copy bank reference key to clipboard', | ||
| 'woocommerce-payments' | ||
| ) } | ||
| title={ __( | ||
| 'Copy to clipboard', | ||
| 'woocommerce-payments' | ||
| ) } | ||
| > | ||
| <i></i> | ||
| </button> | ||
| </> | ||
| ) : ( | ||
| 'N/A' | ||
| ) } | ||
|
There was a problem hiding this comment. Can we make a custom component that encapsulates all this behaviour, e.g. That component would take care of:
Then, we could just render that component alongside all the others in the table header: <SummaryList>
<SummaryItem … />
<SummaryItem … />
<PayoutBankDetails />
</SummaryList>There was a problem hiding this comment. As per discussion during our pair coding session:
There was a problem hiding this comment. @haszari - We discussed adding a smooth transition during our pair coding session. We are currently using SVG as I checked a workaround of rendering separate icons and making them appear and disappear with transitions, but not sure if the added complexity was worth it. Hence have not indulged in the transition animation for now. |
||
| </> | ||
| } | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="wcpay-deposit-overview"> | ||
| { deposit.automatic ? ( | ||
| <Card className="wcpay-deposit-automatic"> | ||
| <ul> | ||
| { DepositDateItem() } | ||
| <li className="wcpay-deposit-amount"> | ||
| { formatExplicitCurrency( | ||
| deposit.amount, | ||
|
|
@@ -172,7 +226,7 @@ export const DepositOverview: React.FC< DepositOverviewProps > = ( { | |
| } | ||
| > | ||
| { () => [ | ||
| DepositDateItem(), | ||
| <SummaryItem | ||
| key="depositAmount" | ||
| label={ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,55 @@ | |
| font-size: 20px; | ||
| line-height: 28px; | ||
| } | ||
|
|
||
| .wcpay-summary__item-detail { | ||
| color: $dark-gray-500; | ||
|
|
||
| @include breakpoint( '<480px' ) { | ||
| word-wrap: break-word; | ||
|
There was a problem hiding this comment. Is this useful at all screen sizes? There was a problem hiding this comment. Yup it is. Thanks for catching that. I will remove the media query. |
||
| } | ||
|
|
||
| .woopayments-payout-bank-reference-key { | ||
| font-family: monospace; | ||
| } | ||
|
|
||
| .woopayments-copy-button-bank-reference-key { | ||
| line-height: 1.2em; | ||
| display: inline-flex; | ||
| background: transparent; | ||
| border: none; | ||
| border-radius: 0; | ||
| vertical-align: middle; | ||
| font-weight: normal; | ||
| cursor: pointer; | ||
| color: inherit; | ||
| margin-left: 2px; | ||
| align-items: center; | ||
|
|
||
| i { | ||
| display: block; | ||
| width: 1.2em; | ||
| height: 1.2em; | ||
| mask-image: url( 'assets/images/icons/copy.svg?asset' ); | ||
| mask-size: contain; | ||
| mask-repeat: no-repeat; | ||
| mask-position: center; | ||
| background-color: currentColor; | ||
|
|
||
| &:hover { | ||
| opacity: 0.7; | ||
| } | ||
|
|
||
| &:active { | ||
| transform: scale( 0.9 ); | ||
| } | ||
| } | ||
|
|
||
| &.state--copied i { | ||
| mask-image: url( 'assets/images/icons/check-green.svg?asset' ); | ||
| } | ||
|
There was a problem hiding this comment. There's a lot of css (and some React behaviour) for this little copy-to-clipboard button. Could we package it into a little component? The button is nicely general-purpose so is a good candidate for an abstraction. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| .wcpay-deposit-fee { | ||
|
|
@@ -52,13 +101,21 @@ | |
| margin: 0; | ||
| list-style-type: none; | ||
|
|
||
| @include breakpoint( '<660px' ) { | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| .woocommerce-summary__item { | ||
| border-bottom: 0; | ||
| background-color: inherit; | ||
| color: inherit; | ||
| .woocommerce-summary__item-label:hover { | ||
| color: inherit; | ||
| } | ||
|
|
||
| @include breakpoint( '<660px' ) { | ||
| border-right: none; | ||
| } | ||
| } | ||
|
|
||
| .wcpay-deposit-amount { | ||
|
|
@@ -67,6 +124,13 @@ | |
| text-align: right; | ||
| font-size: 36px; | ||
| line-height: 82px; | ||
|
|
||
| @include breakpoint( '<660px' ) { | ||
| padding: 24px; | ||
| line-height: 36px; | ||
| text-align: left; | ||
| border-top: 1px solid $studio-gray-5; | ||
| } | ||
|
There was a problem hiding this comment. What are we tweaking for smaller (<660px) screens? Should we make that behaviour the default, and progressively enhance on larger screens (i.e. mobile first)? There was a problem hiding this comment. We are making the font size smaller, making it left aligned and adding a border on top. The layout by itself - overall is desktop first. The columns have a sideways scroll. My gut feel is that merchants would mostly use a tablet or a laptop to check WP admin. Hence would prefer keeping it as-is, but open to revisiting that! The padding property within the media query is redundant though, removed it - 8bda02c. There was a problem hiding this comment.
Why? Ideally the style changes for smaller screens are minimal (so it's easy to maintain), this sounds like a lot! If you are adding different behaviour for different screen sizes I'd recommend including details in the PR description – I see you have a screenshot, but even better if you explain why the changes are needed. There was a problem hiding this comment.
I have added a note on why we are doing the CSS change, in the PR description
|
||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Does this align with the types for the component we are overriding (
SummaryNumberfrom Woo core?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.
The original SummaryItemNumber ( child within the summary list ) , does not have a detail prop. It was introduced in the customized
SummaryListItemfor the Deposit Overview header.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.
Unrelated to this PR, but I think we should avoid inheriting/overriding components like this. We risk depending on implementation details, so our plugin is sensitive to changes in Woo core.
Ideally we'd either: