-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Restore auto-resizing of the code console input prompts #17819
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for making a pull request to jupyterlab! |
Do people think this should be put behind a setting, so users can opt-out of the new behavior? |
Thinking more about this, it's probably fine without the setting, as the behavior in this PR feels a little bit more natural in the end. |
Made a couple of tweaks to the UX. Overall this PR now brings the following improvements:
jupyterlab-code-console-prompt-resize.mp4 |
The failing UI tests are tracked in #17820 ![]() Otherwise, marking as ready, so folks can try the UX (hopefully Binder will be back soon). |
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.
Thanks, @jtpio! Confirming that this looks good and works well from my end 👍🏻 Besides the NumPy website PR, I also highlighted this issue in jupyterlite/jupyterlite-sphinx#290, where I had previously attempted to (incorrectly) fix the problem in the JupyterLite codebase.
Do people think this should be put behind a setting, so users can opt-out of the new behavior?
Thinking more about this, it's probably fine without the setting, as the behavior in this PR feels a little bit more natural in the end.
This was reported as a regression in 4.4 by some users, so looking into fixing it for 4.5.
I agree; this PR is definitely a nice restoration of the original behaviour and shouldn't need to be opted out of.
I've also added a few comments below, and I hope they are helpful somewhat:
return; | ||
} | ||
|
||
const remainingHeight = totalHeight - inputHeight; |
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, what would happen if inputHeight
exceeds totalHeight
, i.e., for very tall content? Should we have something like const maxInputHeight = Math.min(inputHeight, totalHeight * 0.8)
here, or does SplitPanel
clamp negative values to 0
?
If yes, we could also consider if we want to refactor this math into a _calculateSizes
function or similar, as I think _adjustSplitPanelForInputGrowth
alone is doing quite a lot :)
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.
Normally it should not be possible for inputHeight
to be greater than totalHeight
, since totalHeight
corresponds to the height of the SplitPanel
and the input widget is one of the splits.
// Detect height changes | ||
if (promptCell.node) { | ||
this._promptResizeObserver = new ResizeObserver(() => { | ||
if (!this._hasManualResize) { | ||
this._resetInputSize(); | ||
} | ||
requestAnimationFrame(() => { | ||
this._adjustSplitPanelForInputGrowth(); | ||
}); | ||
}); | ||
this._promptResizeObserver.observe(promptCell.node); | ||
} | ||
|
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.
Not sure about any performance considerations or what the previous implementation was like, but would this cause a lot of redraws unless we debounce it?
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 think the previous implementation was just using flex
, so it was offloaded to the browser, but otherwise, yeah, I was thinking about adding some debouncing to this.
The main idea is that we need to catch any change to the height of the prompt cell, which happens most of the time when a user adds a new line.
@@ -999,6 +1116,8 @@ export class CodeConsole extends Widget { | |||
private _focusedCell: Cell | null = null; | |||
private _translator: ITranslator; | |||
private _splitPanel: SplitPanel; | |||
private _promptResizeObserver: ResizeObserver | null = null; | |||
private _hasManualResize = false; |
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.
private _hasManualResize = false; | |
private _hasManualResize: boolean = false; |
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 is not needed as typescript can infer this type
References
Fixes #17597
Also reported in:
This was reported as a regression in 4.4 by some users, so looking into fixing it for 4.5.
Code changes
Use a
ResizeObserver
to check changes in height for the input prompt cell to automatically set the split relative sizes.This only applies when
promptCellPosition
is set tobottom
ortop
.User-facing changes
This PR now brings the following improvements:
bottom
andtop
)jupyterlab-code-console-prompt-resize.mp4
Backwards-incompatible changes
None