-
-
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
Pulled notebook export UI into separate extension so it can be disabled easily #10094
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
isEnabled | ||
}); | ||
|
||
if (mainMenu) { |
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 this check happen after getting the response from the server?
Otherwise if the menu is not provided, it looks like the command would not be added to the command palette either? (the if (palette)
check below)
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.
In the original code, this section (lines 399-442) is all bundled into populateMenus, which is only called after checking for if (mainMenu)
in its parent function.
jupyterlab/packages/notebook-extension/src/index.ts
Lines 951 to 962 in c1f0b02
// Add main menu notebook menu. | |
if (mainMenu) { | |
populateMenus( | |
app, | |
mainMenu, | |
tracker, | |
services, | |
translator, | |
palette, | |
sessionDialogs | |
); | |
} |
That's why I put the check here before anything gets added--I don't have context on the original code so I'm not sure if there's a specific reason it's all bundled together. Should it be different here after pulling it out?
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.
Ah right, thanks for looking it up 👍
Not sure there was a specific reason indeed to have it that way. It just caught attention since usually the menu and palettes are handled as two independent components.
But it would be totally fine to pull it out in a separate change too.
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.
@jtpio just moved it down! Let me know if you think that's a good spot for it, or if I should reorganize it differently.
Looks like there are a few cancelled checks, closing and reopening to make it re-run the checks |
@jtpio it seems the jupyterlab/tests tests keep timing out (they're running over the 30 min mark). Everything that finished running has passed however, including the tests that finished running in jupyterlab/tests--should I be concerned or is this ok? Edit: nvm, looks like everything is passing 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.
Thanks!
* Add 'merge cell above/below' commands with shortcuts * Add tests for mergeCells with mergeAbove = true * Lint * Update after #10094
References
Addresses #5274. Technically, export isn't the exact same as download, but I figured if a user wants to disable download it's highly likely they'll want to disable export as well.
Code changes
User-facing changes
No user-facing changes with the extension enabled as is. If @jupyterlab/notebook-extension:export is disabled (the ID for the new plugin), then the file menu and command palette will no longer show any export commands:
Backwards-incompatible changes
None