-
Notifications
You must be signed in to change notification settings - Fork 407
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
Fix some test and non-test warnings #5294
Fix some test and non-test warnings #5294
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5294 +/- ##
==========================================
- Coverage 88.61% 88.61% -0.01%
==========================================
Files 308 308
Lines 28125 28122 -3
Branches 7621 7620 -1
==========================================
- Hits 24923 24920 -3
Misses 2987 2987
Partials 215 215 ☔ View full report in Codecov by Sentry. |
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 for fixing the warnings! Looks good to me, but let me know what you think about the comment below. It's fine by me if you choose to go with the current approach too.
// As of React 18, If the action dispatch isn't delayed, the component will | ||
// get rerendered with the redux changes before the react state changes are | ||
// applied. As a result CurrentProfileUploadedInformationLoader could reset | ||
// the profile information faster than this metaInfoPanelState was applied, | ||
// and we were getting an error in _renderMetaInfoButton. | ||
// Using requestAnimationFrame ensures that this component's state | ||
// is properly applied before changing the redux state. Note that if we were | ||
// using a function component, we could use useLayoutEffect instead. | ||
// Note that this might not be necessary in future versions of React or | ||
// React-Redux, so be sure to revisit this choice in the future. | ||
requestAnimationFrame(() => { | ||
this.props.profileRemotelyDeleted(); | ||
}); |
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.
Hm this is a bit hacky, but if it works it's okay for me I guess. Do we always know for sure that requestAnimationFrame will be enough for it to update the state?
Another solution might be to have a componentDidUpdate
that checks if the metaInfoPanelState
state has changed, and calls profileRemotelyDeleted
only if it's changed.
48da62f
to
61f1010
Compare
Hey @canova , I updated the code like we discussed, this looks better to me, thanks for the feedback! |
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.
Looks good to me! Also tested it and it works well, thanks!
This fixes 2 warnings:
button.click
instead offireEvent.click
. The former isn't wrapped byact
while the latter is.