-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix (some) test suite warnings #3221
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
Conversation
Fixes the test suite warnings that are unrelated to the class -> functional conversion
Release EnvironmentsThis Environment is provided by Release, learn more! 🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-71a56e04d9 |
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.
This is awesome! Thanks so much for working on this!
Pinging at @nahbee10 to see if she has any additional thoughts as well—otherwise I think I'm going go ahead with merging this in tomorrow!
I'm going to go ahead with merging this in, but please feel free to note if any additional thoughts comes up! |
Yay! thanks rachel!! : ) |
render( | ||
<ErrorModal type="staleSession" closeModal={jest.fn()} service="google" /> | ||
); |
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.
Personally I would say that the more logical fix for the PropTypes error would be to make it so that the service
is not a required prop. It's only needed when type="oauthError"
and it's not used in any other circumstance so it's kind of weird that it's always required. (I'm not sure if PropTypes has a way to require it depending on the value of the type
. The "correct" thing would be to require the service
if and only if the type
is "oauthError" but that might be getting too complicated.)
Fixes the test suite warnings that are unrelated to the React lifecycle, which I assume will be resolved as a side effect of the ongoing class -> functional conversion.
Changes:
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123