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
Refactor controls into separate components #59
Refactor controls into separate components #59
Changes from 8 commits
8dd4b87
f3407e3
a9bc3c7
8b93921
7626608
3acdf47
836b0ea
b8836ae
97e508b
730bfc7
4e5f8f6
b47e0f0
c87b62d
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.
nit: remove
console.log
unless you want to keep it here 🤣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.
Good point. I should make a whole PR to clean up our logging or at least move it to debug 😵💫.
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.
Is this an example of a dumb component? I.e. state is passed through, it uses it for rendering/presentation, and it calls the necessary setters. But it doesn't contain any logic/effects of its own?
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 I think so! Now that you mention that though I feel perhaps this should take (optional?) props for onClick, etc. instead of requiring specific value setters. What do you think? I can try to look if there's some convention for this.
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.
As a general rule, I tend to prefer avoiding optionals if possible. Do you think this might make it easier to reuse?
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.
Yes, and it would be more flexible. The parent (smart component) would be able to take additional actions in onClick aside from just setting a value.
My thinking on being optional is that it would be closer to how a basic
<Button/>
might work, but I'm okay either way on that. I don't think they need to be optional without a specific use case in mind.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 looks really good for a "dumb" component, nice job 🫡
as for whether a prop should be optional, I think it entirely depends on if the component is meant to work in a context where it doesn't need that prop
for example, buttons can be used for submitting forms using the
type="submit"
prop, so it wouldn't need an onclick event handler in that context. additionally, Material UI buttons can also function as links, so it's not required if passing in ahref
prop: