Teacher Tool: Make Splitter Functional #9887
Conversation
…ks/teachertool/splitter_functionality
…n on the splitter
| @@ -5,20 +5,115 @@ import { classList } from "react-common/components/util"; | |||
| interface IProps { | |||
| className?: string; | |||
| split: "horizontal" | "vertical"; | |||
| defaultSize: number | string; | |||
| defaultSize: number | string; // The size to reset to when double clicking the splitter. | |||
| startingSize?: number | string; // The size to use initially when creating the splitter. Defaults to `defaultSize`. | |||
There was a problem hiding this comment.
I'm not sure I understand the benefit of having this extra parameter. I can understand that the parent of the SplitPane should be able to declare the default size of the panes, but I think starting size is something that can be determined inside SplitPane.
I think just having defaultSize is all we need, and then when first setting the size in the useState, you can just have the getLastSplitPosition call as the lastSavedPosition variable and the useState are essentially doing the same thing right now.
There was a problem hiding this comment.
Oh, I see. You're trying to keep the state/service stuff outside of SplitPane to make it easier to move in the future. I wonder if saving the size in localstorage is something you just want to bring with this, though.
There was a problem hiding this comment.
I don't think we should have this class interface with local storage. The caller would still need to pass in the local storage key to use, which would feel a little weird, but more importantly, if we re-use it somewhere else, that behavior may not be desirable. We may not want to save it at all, or we may want to save it somewhere else, like an account-level setting for example.
There was a problem hiding this comment.
Makes sense. Can lastSavedSplitPosition in MainPanel just be const lastSavedSplitPosition = getLastSplitPosition(), then? Or we can even just get rid of the variable and pass the function call into startingSize.
| ? `${((clientX - containerRect.left) / containerRect.width) * 100}%` | ||
| : `${((clientY - containerRect.top) / containerRect.height) * 100}%`; | ||
|
|
||
| setIsResizing(true); // Do this here rather than inside startResizing to prevent interference with double click detection. |
There was a problem hiding this comment.
It probably doesn't make that much of a difference, but is there a reason you don't do this at the start of the if statement?
There was a problem hiding this comment.
I just thought it looked nice to keep the set-state calls together. Do you think we should move it up?
There was a problem hiding this comment.
My initial thought is yes, we should move it up because we want to make sure that onResizeEnd doesn't get called while the container is actively resizing, but I don't know if that case could happen or if it would be the worst thing if it did. It also might be the case that putting setResizing above the newSize definition would cause more problems with the useEffect. It's just one of those React things that I'm a bit fuzzy on.
There was a problem hiding this comment.
It's common to use ResizeObserver in these scenarios. Did you consider using ResizeObserver here before going with this solution?
There was a problem hiding this comment.
I moved it up just in case.
As for the ResizeObserver, I'm not sure that'd help much in this case. I think it's actually preferable to react to the mouse up/down/move events, which feel closer to user's intent than trying to deduce from resize observations.
|
|
||
| const leftStyle: React.CSSProperties = { flexBasis: size }; | ||
| if (split === "vertical") { | ||
| leftStyle.minWidth = leftMinSize; |
There was a problem hiding this comment.
Do we need to make the leftMinSize and rightMinSize props required? If they aren't set, I think these calculations will fail sine we don't have default values for leftMinSize and rightMinSize.
There was a problem hiding this comment.
Hmm, I don't think we should allow them to be fully unset, since it's possible to get into a state where the splitter goes offscreen and then you can never get out of that state, even when you refresh the page.
We could have a default value, but when in doubt, I think it's simpler to just have the caller tell us what these values should be. The caller will always have more context than this component does, and it's not too burdensome to include.
There was a problem hiding this comment.
Yes, I agree, but right now these props are optional: https://github.com/microsoft/pxt/pull/9887/files#diff-5c44e5a8b91e45e13e21ff9f1a12da3634d9282699fb7a757190de64abcff78dR13-R14. I think they should just be required props to avoid possible problems.
This makes it so the splitter can actually be used to resize the windows. I've also made it so you can double-click the split to reset it to its original position.
I intentionally did not tie the split position into app state, so the control will be more reusable in the future, if we move it to react common.
It'd be nice to merge this and the existing VerticalResizeContainer, but they've approached the problem from two somewhat different angles that makes the refactor a bit messy. The VerticalResizeContainer is a single window that can have its height changed, but it doesn't care what's shrinking/expanding below it. Meanwhile, this is a container for two different panels that manages both sides. In the future, I think it'd make sense to move this SplitPanel into React Common and take anything that's using the VerticalResizeContainer and have it use the SplitPanel instead, but I wanted to save that for another change.
Upload Target: https://makecode.microbit.org/app/50d4b45ccb39db5b42fee1e5cacc60776082aa3c-32ad1744d7--eval