Add "Render all markdown cells" command #6029
Conversation
|
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
rahulpshah
changed the title
There was a problem hiding this comment.
Thanks @rahulpshah! A few comments, but overall this looks good.
packages/mainmenu/src/run.ts
Outdated
| @@ -74,6 +74,8 @@ export namespace IRunMenu { | |||
| */ | |||
| runAll?: (widget: T) => Promise<void>; | |||
|
|
|||
| runAllMarkdown?: (widget: T) => Promise<void>; | |||
There was a problem hiding this comment.
The ICodeRunner interface is meant to expose a way for many different types of things that run code to hook into the "Run" menu. In this case, I think this command is pretty specific to notebooks, and not a generic "code-running" activity. So I'd recommend adding the command directly to the menu, rather than through the codeRunner.
| @@ -119,6 +119,8 @@ namespace CommandIDs { | |||
|
|
|||
| export const runAllBelow = 'notebook:run-all-below'; | |||
|
|
|||
| export const runAllMarkdown = 'notebook:run-all-markdown'; | |||
There was a problem hiding this comment.
Maybe better to name it render-all-markdown/renderAllMarkdown for consistency with the label?
packages/notebook/src/actions.tsx
Outdated
| notebook.activeCellIndex = index; | ||
| } | ||
| }); | ||
| if (notebook.widgets[notebook.activeCellIndex].model.type !== 'markdown') { |
There was a problem hiding this comment.
You can make this a bit simpler by accessing notebook.activeCell.model.type
| @@ -2062,6 +2076,12 @@ function populateMenus( | |||
| () => void 0 | |||
| ); | |||
| }, | |||
| runAllMarkdown: current => { | |||
There was a problem hiding this comment.
This is where you would add a group containing the renderAllMarkdown to the "Run" menu, rather than adding it to the codeRunners.
There was a problem hiding this comment.
Sorry for the slow review turnaround @rahulpshah, this is looking good. A few more comments oriented around cleanup, then I think we should be good to go.
| @@ -92,6 +92,8 @@ export namespace CommandIDs { | |||
|
|
|||
| export const runAll = 'runmenu:run-all'; | |||
|
|
|||
| export const runAllMarkdown = 'runmenu:run-all-markdown'; | |||
There was a problem hiding this comment.
I don't think this ID should be here at all -- there is no 'runmenu:run-all-markdown' command defined as far as I see.
| const runAllGroup = [ | ||
| CommandIDs.runAll, | ||
| CommandIDs.restartAndRunAll, | ||
| CommandIDs.runAllMarkdown |
| // Add a renderAllMarkdown group to the run menu. | ||
| const renderAllMarkdown = [ | ||
| CommandIDs.renderAllMarkdown, | ||
| CommandIDs.run, |
There was a problem hiding this comment.
Can you remove run and runInConsole here? The first is redundant with existing menu items, and the second should probably have its own discussion. I think it's fine for renderAllMarkdown to have its own menu group.
|
Hi @ian-r-rose, I have made the changes you requested but I am unsure why the windows build is failing. Any pointers? |
|
@rahulpshah That's probably due to the flakiness of our CI. I can try to restart it to see if it passes. |
|
Looks great, thanks @rahulpshah! |
jasongrout
changed the title
lock
bot
added
the
status:resolved-locked
Fixes #6017 - Adds an option to render markdown cells.
On clicking the button, It renders all the markdown cells in the notebook. The PR makes sure that the active cell is not executed if it is not markdown.
Run Menu:
Sample Notebook
Before:
After:
@ian-r-rose Let me know your thoughts on this PR? Is there something that needs to be added?