-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add the command to toggle the virtual scrollbar to the palette #16322
Conversation
Thanks for making a pull request to jupyterlab! |
Should we adjust the label to "Toggle Virtual Scrollbar" when used from palette? Or maybe in general? In that case this would need to target 4.3.x to avoid breaking translations. |
It looks like other commands don't have the word "Toggle" in their label: However some commands use the word "Show", such as "Show Line Numbers", which then uses a checkbox when the lines are displayed: Would it make sense to do something similar for the virtual scrollbar? |
Yes, it makes sense yo me. It is more of a suggestion on making it easier for user to understand what this command is supposed to do, I do not want to block merging on this here, and if you wanted to backport this to 4.2.x the label could be adjusted in a follow up which would target 4.3.0 |
Maybe there is no hurry to get this in a 4.2.x release and we can take the time to adjust the label correctly. |
909ae90
to
2084370
Compare
@@ -3554,6 +3554,10 @@ function addCommands( | |||
(settings?.composite.windowingMode === 'full' ?? false); | |||
return enabled; | |||
}, | |||
isToggled: () => { | |||
const current = tracker.currentWidget; |
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.
Should we use getCurrent
here for consistency?
const current = tracker.currentWidget; | |
const current = getCurrent(tracker, shell, args); |
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 this was the case at some point while iterating on this PR, but it was creating some issues because of the activate
logic in that function, which was preventing switching to another widget in the dock panel:
jupyterlab/packages/notebook-extension/src/index.ts
Lines 2145 to 2149 in 56a0121
const activate = args['activate'] !== false; | |
if (activate && widget) { | |
shell.activateById(widget.id); | |
} |
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.
Ok, thanks for the clarification!
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!
References
This makes it easier to toggle the virtual scrollbar with the keyboard only.
Code changes
Add the
notebook:toggle-virtual-scrollbar
command to the command paletteUser-facing changes
Before
After
Backwards-incompatible changes
None