Skip to content
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 "Render all markdown cells" command #6029

Merged

Conversation

@rahulpshah
Copy link
Contributor

@rahulpshah rahulpshah commented Feb 24, 2019

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:

screen shot 2019-02-23 at 9 02 35 pm

Sample Notebook
Before:
screen shot 2019-02-23 at 9 07 34 pm
After:
screen shot 2019-02-23 at 9 07 44 pm

@ian-r-rose Let me know your thoughts on this PR? Is there something that needs to be added?

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Feb 24, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@rahulpshah rahulpshah changed the title Feature/jp 6017 render mkd cells Add "Render all markdown cells" command, or automatically render markdown Feb 24, 2019
@jasongrout jasongrout added this to the 1.0 milestone Feb 24, 2019
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks @rahulpshah! A few comments, but overall this looks good.

@@ -74,6 +74,8 @@ export namespace IRunMenu {
*/
runAll?: (widget: T) => Promise<void>;

runAllMarkdown?: (widget: T) => Promise<void>;
Copy link
Member

@ian-r-rose ian-r-rose Feb 28, 2019

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';
Copy link
Member

@ian-r-rose ian-r-rose Feb 28, 2019

Maybe better to name it render-all-markdown/renderAllMarkdown for consistency with the label?

notebook.activeCellIndex = index;
}
});
if (notebook.widgets[notebook.activeCellIndex].model.type !== 'markdown') {
Copy link
Member

@ian-r-rose ian-r-rose Feb 28, 2019

You can make this a bit simpler by accessing notebook.activeCell.model.type

@@ -2062,6 +2076,12 @@ function populateMenus(
() => void 0
);
},
runAllMarkdown: current => {
Copy link
Member

@ian-r-rose ian-r-rose Feb 28, 2019

This is where you would add a group containing the renderAllMarkdown to the "Run" menu, rather than adding it to the codeRunners.

@afshin afshin requested review from afshin and tgeorgeux Mar 27, 2019
Copy link
Member

@ian-r-rose ian-r-rose left a 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';
Copy link
Member

@ian-r-rose ian-r-rose Apr 1, 2019

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
Copy link
Member

@ian-r-rose ian-r-rose Apr 1, 2019

This line can be removed.

// Add a renderAllMarkdown group to the run menu.
const renderAllMarkdown = [
CommandIDs.renderAllMarkdown,
CommandIDs.run,
Copy link
Member

@ian-r-rose ian-r-rose Apr 1, 2019

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.

@rahulpshah
Copy link
Contributor Author

@rahulpshah rahulpshah commented Apr 2, 2019

Hi @ian-r-rose, I have made the changes you requested but I am unsure why the windows build is failing. Any pointers?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 2, 2019

@rahulpshah That's probably due to the flakiness of our CI. I can try to restart it to see if it passes.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 5, 2019

Looks great, thanks @rahulpshah!

@ian-r-rose ian-r-rose merged commit cfa683f into jupyterlab:master Apr 5, 2019
9 checks passed
@rahulpshah rahulpshah deleted the feature/JP-6017-render-mkd-cells branch Apr 11, 2019
@jasongrout jasongrout changed the title Add "Render all markdown cells" command, or automatically render markdown Add "Render all markdown cells" command Apr 26, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants