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
Teacher Tool: Make Splitter Functional #9887
Teacher Tool: Make Splitter Functional #9887
Changes from all commits
cf40870
1de9ef7
90b2da8
85a0b33
e09fd19
c1e6943
232a9be
99b0c49
fff61e4
e2a6c91
f739581
5861ba0
17a76f0
bbc0130
7230ad5
2dc8a7a
52c6203
caea1dd
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.
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 insideSplitPane
.I think just having
defaultSize
is all we need, and then when first setting the size in theuseState
, you can just have thegetLastSplitPosition
call as thelastSavedPosition
variable and theuseState
are essentially doing the same thing right now.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Can
lastSavedSplitPosition
inMainPanel
just beconst lastSavedSplitPosition = getLastSplitPosition()
, then? Or we can even just get rid of the variable and pass the function call intostartingSize
.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.
Do we need to make the
leftMinSize
andrightMinSize
props required? If they aren't set, I think these calculations will fail sine we don't have default values forleftMinSize
andrightMinSize
.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.