Refactor controls into separate components #59
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I'm marking this ready for review, given it has been helpful for me in #60. |
aganders3
changed the title
| } | ||
|
|
||
| export default function PlaybackControls(props: PlaybackControlsProps) { | ||
| return ( |
There was a problem hiding this comment.
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.
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.
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.
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.
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 a href prop:
<Button href="#text-buttons">Link</Button>
|
I also get a healthy horizontal scroll bar in Chrome and Safari regardless of window size or display. 
|
There was a problem hiding this comment.
This looks good enough to merge now. Overall, it feels a lot more organized and understandable than main too. I just had a couple of minor optional comments.
Also, good catch on finding the problem with using refs in useEffect! Ideally, I'd still like PointCanvas to be non-nullable in which case it doesn't need to be React state at all, but still not sure how best to do that, so let's leave that for the future.
|
Thanks! I'll merge this shortly and rebase #60 on main after.
I agree - I'll give this some thought. I think there's probably a way to get rid of the onMount style useEffects we have. Maybe this can be fixed as part of changing the selection method. |
aganders3
force-pushed
the
refactor-component-structure
branch
from
be507a9 to
c87b62d
Compare
| } | ||
|
|
||
| export default function PlaybackControls(props: PlaybackControlsProps) { | ||
| return ( |
There was a problem hiding this comment.
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 a href prop:
<Button href="#text-buttons">Link</Button>| const numTimes = props.trackManager?.points.shape[0] ?? 0; | ||
| const trackLengthPct = numTimes ? (props.trackHighlightLength / 2 / numTimes) * 100 : 0; | ||
|
|
||
| console.log("trackLengthPct: %s", props.trackManager?.maxPointsPerTimepoint); |
There was a problem hiding this comment.
nit: remove console.log unless you want to keep it here 🤣
There was a problem hiding this comment.
Good point. I should make a whole PR to clean up our logging or at least move it to debug 😵💫.
This is a big refactor, but I think it will help with implementing @clarsen-czi's designs.
I think this has feature-parity with
main, but could definitely use some testing. Obviously the appearance is slightly different, but if anything I think this already removes some irrelevant code (updating render width/height) and is a bit closer to the behavior we want (canvas fills its allotted space).To make things more palatable I propose refining and merging this, then we can use the mui classes like Grid or Drawer to move the control groups into place for the new design.