-
Notifications
You must be signed in to change notification settings - Fork 0
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
Rithm add prop types #16
Conversation
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.
See comments.
Also, remove the testing stuff, unless there's a pressing reason to keep it.
package.json
Outdated
"pretty-quick": "^1.11.1", | ||
"pretty-quick": "^1.8.0", | ||
"prop-types": "^15.7.2", | ||
"react-testing-library": "^5.4.2", |
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.
Remove
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.
Removed
src/components/ErrorToast.js
Outdated
@@ -10,6 +11,10 @@ const ErrorToast = ({ message }) => { | |||
); | |||
}; | |||
|
|||
ErrorToast.propTypes = { |
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.
Be consistent -- propTypes before defaultProps or vice versa, not one or the other.
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.
Fixed
src/components/SidebarButton.test.js
Outdated
@@ -6,7 +6,7 @@ afterEach(cleanup); | |||
|
|||
describe('<SidebarButton/>', () => { | |||
it('renders without crashing', () => { | |||
global.renderWithRedux(<SidebarButton />); | |||
global.renderWithRedux(<SidebarButton icon="icon" />); |
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.
What does this addition do?
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.
removed, no longer needed with default props added
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.
Issue [#41]
I added prop types and default props to the following components:
App.js
Burst.js
ErrorToast.js
Frame.js
Preview.js
Settings.js
Sidebar.js
SidebarButton.js
SidebarButtonWithBadge.js
Default props for connected components have been set to match initial state from the store, with there mapDispatchToProps functions have been set as simple arrow functions, i.e. () => {}. For non connected components, the default props have been set to match the properties applied where those components are rendered, i.e. where is rendered within
thanks!