-
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
Payouts: Show bank reference key on payout details page #9886
Changes from all commits
33208a5
98333d5
1939d4e
2d5a129
e0f0dd6
aea1f35
a935636
6386ab8
3ace18a
d318c33
735fe94
e96ba95
ed00ff5
d1339f9
292bfd9
b503845
8bda02c
25b87b3
294a44d
34790c9
21b2e01
05ee6ca
4a4720a
a159495
aa7eb1b
af176bd
0d26f76
5c3c6aa
feb2746
3b4bcc9
8a4f35e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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 |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import React, { useState, useEffect, useRef } from 'react'; | ||
import { __ } from '@wordpress/i18n'; | ||
import classNames from 'classnames'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import './style.scss'; | ||
|
||
interface CopyButtonProps { | ||
/** | ||
* The text to copy to the clipboard. | ||
*/ | ||
textToCopy: string; | ||
|
||
/** | ||
* The label for the button. Also used as the aria-label. | ||
*/ | ||
label: string; | ||
} | ||
|
||
export const CopyButton: React.FC< CopyButtonProps > = ( { | ||
textToCopy, | ||
label, | ||
} ) => { | ||
// useRef() is used to store the timer reference for the setTimeout() function. | ||
const timerRef = useRef< NodeJS.Timeout | null >( null ); | ||
|
||
// useEffect() is used to clear the timer reference when the component is unmounted. | ||
useEffect( () => { | ||
return () => { | ||
if ( timerRef.current ) { | ||
clearTimeout( timerRef.current ); | ||
} | ||
}; | ||
}, [] ); | ||
|
||
const [ copied, setCopied ] = useState( false ); | ||
|
||
const copyToClipboard = () => { | ||
navigator.clipboard.writeText( textToCopy ); | ||
setCopied( true ); | ||
timerRef.current = setTimeout( () => { | ||
setCopied( false ); | ||
}, 2000 ); | ||
}; | ||
|
||
return ( | ||
<button | ||
type="button" | ||
className={ classNames( 'woopayments-copy-button', { | ||
'state--copied': copied, | ||
} ) } | ||
aria-label={ label } | ||
title={ __( 'Copy to clipboard', 'woocommerce-payments' ) } | ||
onClick={ copyToClipboard } | ||
> | ||
<i></i> | ||
</button> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
.woopayments-copy-button { | ||
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' ); | ||
background-color: $studio-green-50; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`CopyButton renders the button correctly 1`] = ` | ||
<div> | ||
<button | ||
aria-label="Copy bank reference ID to clipboard" | ||
class="woopayments-copy-button" | ||
title="Copy to clipboard" | ||
type="button" | ||
> | ||
<i /> | ||
</button> | ||
</div> | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/** @format **/ | ||
|
||
/** | ||
* External dependencies | ||
*/ | ||
import React from 'react'; | ||
import { act, render, screen } from '@testing-library/react'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { CopyButton } from '..'; | ||
|
||
describe( 'CopyButton', () => { | ||
it( 'renders the button correctly', () => { | ||
const { container: copyButtonContainer } = render( | ||
<CopyButton | ||
textToCopy="test_bank_reference_id" | ||
label="Copy bank reference ID to clipboard" | ||
/> | ||
); | ||
|
||
expect( copyButtonContainer ).toMatchSnapshot(); | ||
} ); | ||
|
||
describe( 'when the button is clicked', () => { | ||
beforeAll( () => { | ||
jest.useFakeTimers(); | ||
} ); | ||
|
||
afterAll( () => { | ||
jest.useRealTimers(); | ||
} ); | ||
|
||
it( 'copies the text to the clipboard and shows copied state', () => { | ||
render( | ||
<CopyButton | ||
textToCopy="test_bank_reference_id" | ||
label="Copy bank reference ID to clipboard" | ||
/> | ||
); | ||
|
||
const button = screen.queryByRole( 'button', { | ||
name: /Copy bank reference ID to clipboard/i, | ||
} ); | ||
|
||
//Mock the clipboard API | ||
Object.assign( navigator, { | ||
clipboard: { | ||
writeText: jest.fn(), | ||
}, | ||
} ); | ||
|
||
act( () => { | ||
button?.click(); | ||
} ); | ||
|
||
expect( navigator.clipboard.writeText ).toHaveBeenCalledWith( | ||
'test_bank_reference_id' | ||
); | ||
expect( button ).toHaveClass( 'state--copied' ); | ||
|
||
act( () => { | ||
jest.advanceTimersByTime( 2000 ); | ||
} ); | ||
|
||
expect( button ).not.toHaveClass( 'state--copied' ); | ||
} ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,15 @@ | |
font-size: 20px; | ||
line-height: 28px; | ||
} | ||
|
||
.wcpay-summary__item-detail { | ||
color: $dark-gray-500; | ||
word-wrap: break-word; | ||
|
||
.woopayments-payout-bank-reference-id { | ||
font-family: monospace; | ||
} | ||
} | ||
} | ||
|
||
.wcpay-deposit-fee { | ||
|
@@ -52,13 +61,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 +84,12 @@ | |
text-align: right; | ||
font-size: 36px; | ||
line-height: 82px; | ||
|
||
@include breakpoint( '<660px' ) { | ||
line-height: 36px; | ||
text-align: left; | ||
border-top: 1px solid $studio-gray-5; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 (
SummaryNumber
from 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
SummaryListItem
for 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: