Added fullscreen feature with command alt f #16308
Conversation
|
Thanks for making a pull request to jupyterlab! |
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
|
@meeseeksdev tag enhancement bot please update snapshots |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Documentation snapshots updated. |
|
Galata snapshots updated. |
|
@krassowski We've adjusted the code to align more with the existing codebase and cleaned up the uncaught error issue that previously failed "jplm run eslint:typed". If this is acceptable, we'd like to close this and work on full-screening individual elements as a separate issue next, e.g. terminal/notebook/widgets etc as requested in the previous PR that attempted to solve issue #8710 here: bot please update snapshots |
|
Documentation snapshots updated. |
|
Galata snapshots updated. |
|
Just as FYI, you do not need to write down the code changes by file and line range in the PR description (these these quickly get outdated). What I would recommend is to focus on listing major API changes if there are any, e.g. that |
...ntation/debugger.test.ts-snapshots/debugger-stop-on-raised-exception-documentation-linux.png
Outdated
Show resolved
Hide resolved
galata/test/jupyterlab/notebook-run.test.ts-snapshots/notebook-panel-1-jupyterlab-linux.png
Outdated
Show resolved
Hide resolved
packages/application/src/shell.ts
Outdated
| /** | ||
| * Toggle the fullscreen mode based on user inputs | ||
| */ | ||
| setFullscreenMode(): void { |
There was a problem hiding this comment.
Why do we need this method? Could it be handled in the setter of fullscreenMode property instead to minimize the API surface?
packages/application/src/shell.ts
Outdated
| * `jp-mod-fullscreenMode` CSS class. | ||
| */ | ||
| get fullscreenMode(): boolean { | ||
| return this.hasClass('jp-mod-fullscreenMode'); |
There was a problem hiding this comment.
I don't think we should store state which can be modified by the JS API by using a CSS class - this can lead to inconsistent state if an extension or JS in outputs of user executed code manipulate the fullscreen state (for example rise extension for presenting slides could do that).
I would prefer if the getter returned the state derived from the browser API like fullscreenElement. That said, I am not even sure if we need a getter/setter here - if we are not setting the CSS class, then we could get away without extending the public API of LabShell at all, which will make merging this PR much easier.
There was a problem hiding this comment.
We made the changes and removed the getter/setter and directly queried the fullscreenElement value, we also reverted toggle sidebar widget to its original code. Thank you! (:
|
@krassowski it looks like we're failing some tests and unsure what we need to do to fix it, could you help us with this? |
|
Some of the failing tests are just flaky, I can restart these. One screenshot test appears to need updating: bot please update galata snapshots (we may need to revert some spurious updates afterwards) |
|
Galata snapshots updated. |
galata/test/jupyterlab/notebook-run.test.ts-snapshots/notebook-panel-1-jupyterlab-linux.png
Outdated
Show resolved
Hide resolved
galata/test/jupyterlab/notebook-edit.test.ts-snapshots/split-cell-jupyterlab-linux.png
Outdated
Show resolved
Hide resolved
galata/test/jupyterlab/menus.test.ts-snapshots/opened-menu-view-appearance-jupyterlab-linux.png
Outdated
Show resolved
Hide resolved
|
bot please update galata snapshots |
|
Documentation snapshots updated. |
|
Galata snapshots updated. |
|
@krassowski i think we made all the right changes, could you help us with this? |
packages/application/src/shell.ts
Outdated
| /** | ||
| * Toggle the fullscreen mode based on user inputs | ||
| */ | ||
| toggleFullscreenMode(): void { |
There was a problem hiding this comment.
Can I ask for one more thing - to move this method into a function in packages/application-extension/src/index.tsx?
The rationale is a follows: once we add something to LabShell class , any attempt to change it will be potentially breaking for an extension. Since there is further expansion of the fullscreen capabilities requested (to cover making individual elements take the full screen) enhancements, I think it is likely that the implementation may change.
There was a problem hiding this comment.
@krassowski we’ve removed the method and added it to packages/application-extension/src/index.tsx, let us know if it looks good
References:
Fixes #8710 - Add fullscreen option to JupyterLab
Adding the base fullscreen mode functionality in preparation for fullscreen mode expansion to allow individual widgets to enter fullscreen mode for presentation purposes per the requests in PR #10676.
Code Changes:
jupyterlab/packages/application-extension/src/index.tsx
Added Lines 113-114 - Export the toggleFullscreenMode component.
Added Lines 438-454 - The toggleFullscreenMode command component
Added Line 544 - Register the new command as a CommandIDs entry in the palette
jupyterlab/packages/application-extension/schema/commands.json
Added Lines 293-296 - Include keyboard shortcut of Alt + F
Added Lines 50-54 - Include the fullscreen option in the top bar menu
jupyterlab/packages/application/shell.d.ts
Added Lines 282-287 Created the entry to get/set the fullscreenMode css value to the shell div (preparation for individual fullscreen widgets)
User-Facing Changes:
Provides a menu option to enter fullscreen mode and uses Alt + F as the shortcut key to toggle into and out of fullscreen mode.
F11 remains disabled as a means to enter fullscreen mode to maintain current production JupyterLab behavior.
If entering fullscreen using the Alt + F shortcut or button in the view > Fullscreen Mode menu, users can exit with the view > Fullscreen Mode menu option, Alt + F, ESC key, or F11 key, and JupyterLab maintains fullscreen synchronization of the menu item, check box, and CSS styling.
Backwards-incompatible Changes:
None