Reporting: Show bank reference ID on Payout details page #9945
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: +1.35 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
6 tasks
nagpai
changed the title
nagpai
changed the title
Jinksi
reviewed
Dec 18, 2024
Comment on lines
25
to
64
| 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> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Nice work implementing this tricky bit of logic. Using the setTimeout and cleaning up in the useEffect return works well 🙌
I had a thought while looking at this, though – our intention with to show → 2s → hide a CSS change might be better served by a CSS animation. Since CSS animations already have "timer" logic, we can rely on it rather than implement a custom timer, whilst using React state to add and remove the classname.
The onAnimationEnd event handler will also abort if the element is unmounted, so we don't need to handle any cleanup of any timers.
Let me know what you think – I think it is a tidier and less complex approach (less code to understand and maintain), but there are tradeoffs. E.g. it might be less obvious to code readers where the timer is originating from (CSS, rather than JS).
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 }
+ onAnimationEnd={ () => setCopied( false ) }
>
<i></i>
</button>
);And then in the CSS:
&.state--copied {
animation: copy-indicator 2s forwards;
}
@keyframes copy-indicator {
0% {
opacity: 1;
}
95% {
opacity: 1;
}
// a quick fade-out from 1%→0% at the end
100% {
opacity: 0;
}
}There was a problem hiding this comment.
The CSS suggestion is brilliant. I would prefer using CSS to additional JS/ React script. It is much cleaner and easier to maintain. I will try this out and update. Thanks so much!
Jinksi
reviewed
Dec 18, 2024
Jinksi
reviewed
Dec 20, 2024
| expect( button ).toHaveClass( 'state--copied' ); | ||
|
|
||
| act( () => { | ||
| jest.advanceTimersByTime( 2000 ); |
There was a problem hiding this comment.
Ah, so it seems that the jest JSDOM env won't be running CSS animations, but this can be manually triggered using fireEvent:
Suggested change
| jest.advanceTimersByTime( 2000 ); | |
| fireEvent.animationEnd( button ); |
Then we also won't need to override the jest timer setup and teardown.
…out-details-header
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9878
Changes proposed in this Pull Request
Current status
Awaiting some guidance from design on responsive behavior of the additional header component.
The component currently renders like this:
Testing instructions
npm run changelogto add a changelog file, choosepatchto 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