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.
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
Reporting: Show bank reference ID on Payout details page #9945
base: develop
Are you sure you want to change the base?
Reporting: Show bank reference ID on Payout details page #9945
Changes from 36 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
3be4a58
ac2c8a9
17cd80d
9ac8116
d6ab75c
c0d01b0
24dc671
fd301fe
459b83a
83f7407
68eb326
f0cc6bf
e1d5923
7c879f1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nice work implementing this tricky bit of logic. Using the
setTimeout
and cleaning up in theuseEffect
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).
And then in the CSS:
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 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!
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.
Ah, so it seems that the jest JSDOM env won't be running CSS animations, but this can be manually triggered using
fireEvent
:Then we also won't need to override the jest timer setup and teardown.
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.
Thanks Eric!