-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Hide the terminals part of the running sessions status bar item by default #16846
Hide the terminals part of the running sessions status bar item by default #16846
Conversation
Thanks for making a pull request to jupyterlab! |
Awesome! |
4d6d388
to
e190d63
Compare
be76add
to
d3f2c34
Compare
Following @krassowski's comment in #11547 (comment) :
This PR was updated to:
And the UI tests snapshots updated accordingly. Marking it as ready so it can be considered for 4.4.0. |
const showStatusBarItem = settings.get('showStatusBarItem') | ||
.composite as boolean; | ||
if (showStatusBarItem) { | ||
disposable = registerItem(); |
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.
Wouldn't it register the item multiple times once we add more settings? I think it needs a guard to check if the disposable is defined and not disposed
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.
It looks like that would be the case. Maybe we can indeed add such guard to anticipate the case where new settings would be added 👍
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.
Or maybe we can just dispose the item, and recreate it if showStatusBarItem
is true
.
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.
Some minor improvement suggestions
<terminalIcon.react verticalAlign="middle" stylesheet="statusBar" /> | ||
</GroupItem> | ||
) : ( | ||
<div></div> |
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.
Can we return null
or otherwise at least an empty fragment (<></>
) here? See https://react.dev/learn/conditional-rendering#conditionally-returning-nothing-with-null
<div></div> | |
null |
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.
Looks like the use of GroupItem
prevents this:
Type 'Element | null' is not assignable to type 'Element | (Element & string)'.
Type 'null' is not assignable to type 'Element | (Element & string)'.
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 for checking, I've opened #17212
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.
Thank you @jtpio!
Thanks @krassowski for the review! |
References
Fixes #11547
As discussed in #11547, this status bar item is probably not very useful in practice. And hiding it by default would leave more space for other items.
Code changes
User-facing changes
jupyterlab-autohide-terminal-status-bar.mov
Backwards-incompatible changes
None