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 "Restart kernel, re-run whole notebook" button #8024

Merged
merged 2 commits into from Mar 26, 2020
Merged

Add "Restart kernel, re-run whole notebook" button #8024

merged 2 commits into from Mar 26, 2020

Conversation

ryantam626
Copy link
Contributor

@ryantam626 ryantam626 commented Mar 10, 2020

References

#7935

Code changes

  • Add fast forward icon, from the rest of the svg, it looks like it's coming from material UI icons, so I copied fast foward from here, and applied some stylistic changes to keep it consistent to what is already done;
  • Add ToolbarItems.createRestartRunAllButton and register this new button in ToolbarItems.getDefaultItems (I used the implementation of restart and run all command in notebook-extension as reference);
  • Add tests obviously;

User-facing changes

Selection_001
(Note the presence of the fast forward icon)

Backwards-incompatible changes

None.

ryantam626 added 2 commits Mar 10, 2020
We will be using this for this "Restart kernel, re-run whole notebook" button.

Related: #7935
To achieve a more faithful about duplicating the jupyter notebook
toolbar.

Add tests while we are at it.

Related: #7935
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Mar 10, 2020

Thanks for making a pull request to JupyterLab!

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

@github-actions github-actions bot added tag:Design System CSS pkg:notebook pkg:ui-components tag:CSS tag:Testing labels Mar 10, 2020
@jasongrout jasongrout added this to the 2.1 milestone Mar 10, 2020
@jasongrout jasongrout removed this from the 2.1 milestone Mar 12, 2020
@jasongrout jasongrout added this to the 3.0 milestone Mar 12, 2020
@jasongrout jasongrout removed this from the 3.0 milestone Mar 24, 2020
@jasongrout jasongrout added this to the 2.1 milestone Mar 24, 2020
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 25, 2020

Like you mention, we already have a command to restart and run all. Is there a reason to not just use a CommandButton to invoke that command?

Also, CC @tgeorgeux for UX review for the icon. Note that this icon is what is used in the classic notebook these days for this action:
Screen Shot 2020-03-25 at 8 05 19 AM
I personally like the restart and run all icon from the nteract discussion, though (perhaps with the arrow going the other way, though): nteract/nteract#4809 (comment)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 25, 2020

Is there a reason to not just use a CommandButton to invoke that command?

Yes, there is a reason not to use the command. For the notebook, the toolbar is defined in the notebook package, not the notebook extension package, so it does not have access to the command.

Copy link
Contributor

@jasongrout jasongrout left a comment

The code looks and seems to work great. I suggested a few small changes to the text, but other than that, this looks good.

Thanks!

packages/notebook/src/default-toolbar.tsx Show resolved Hide resolved
@ryantam626
Copy link
Contributor Author

@ryantam626 ryantam626 commented Mar 25, 2020

I personally like the restart and run all icon from the nteract discussion

Oh yes, same here! That looks much more intuitive. Let me know if the appropriate SVG can be produced and I can make the changes 👍

I suggested a few small changes to the text

Yep makes sense, will change and squash the relevant commit.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Mar 25, 2020

Next steps here are to address Jason's suggestion and for @tgeorgeux to provide an appropriate icon.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Mar 25, 2020

For the Icon we can use:
--jp-icon-restart-and-run

The code for that SVG is here (Is there an easy way I could just add a file to the PR?)

<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">
<g class="jp-icon3" fill="#616161">
<path d="M13 3C10.6131 3 8.32387 3.94821 6.63604 5.63604C4.94821 7.32387 4 9.61305 4 12H1L4.96 16.03L9 12H6C6 10.1435 6.7375 8.36301 8.05025 7.05025C9.36301 5.7375 11.1435 5 13 5C14.8565 5 16.637 5.7375 17.9497 7.05025C19.2625 8.36301 20 10.1435 20 12C20 13.8565 19.2625 15.637 17.9497 16.9497C16.637 18.2625 14.8565 19 13 19C11.07 19 9.32 18.21 8.06 16.94L6.64 18.36C8.27 20 10.5 21 13 21C15.3869 21 17.6761 20.0518 19.364 18.364C21.0518 16.6761 22 14.3869 22 12C22 9.61305 21.0518 7.32387 19.364 5.63604C17.6761 3.94821 15.3869 3 13 3Z"/>
<path d="M17 12L11 16.5V7.5L17 12Z"/>
</g>
</svg> 

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Mar 25, 2020

@jasongrout the reason I used the arrow going 'backward' was to communicate the kernel going 'back' before it restarts. If that's not clear I can reverse the direction of the arrow.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 25, 2020

If that's not clear I can reverse the direction of the arrow.

I haven't thought about it enough to have an opinion. I didn't even realize the arrows had a standard direction until @ivanov pointed it out to me the other day in that discussion.

Is it easy to post the forward-facing arrow to see what it looks like? The fact that browsers use the forward facing arrow for reload makes me think that might be analogous to restart. I wish we had more examples of this sort of icon.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 25, 2020

Here is macOS:
Screen Shot 2020-03-25 at 2 31 16 PM

(so it isn't a circle arrow, but it is "backwards")

Here is Windows:
Screen Shot 2020-03-25 at 2 35 29 PM

Checking on our Windows 10 computer, it's also a counter-clockwise arrow for reboot.

So I think precedence is there to have a counter-clockwise arrow.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Mar 25, 2020

Here are some examples from Material Design Icons:
Screen Shot 2020-03-25 at 4 43 10 PM
Screen Shot 2020-03-25 at 4 42 59 PM
Screen Shot 2020-03-25 at 4 43 16 PM
Screen Shot 2020-03-25 at 4 43 04 PM

That's where the inspiration came from for the one I made. Only refresh is clockwise, I don't think of what the user is doing here as a refresh as much as a clean start.

I am open to being convinced otherwise, that's just what I am shooting for.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 26, 2020

I am open to being convinced otherwise, that's just what I am shooting for.

Sounds good to me.

@tgeorgeux tgeorgeux merged commit d8c66c0 into jupyterlab:master Mar 26, 2020
10 checks passed
@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Mar 26, 2020

Thanks.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 26, 2020

Are we doing the icon in a different pr?

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Mar 26, 2020

Ah sorry, I spaced, I can open a new PR to change the icon.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 26, 2020

I think it's better as a follow-up PR at this point.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Mar 26, 2020

Yeah, I re-though that and edited my response right after I made it. I'll open a new PR with the comments you had outstanding incorporated, with the new icon included. Would you mind giving it a check for me when I'm done?

Sorry to complicate this 🤦‍♂️

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Mar 26, 2020

New PR sounds great! I am happy to review too. Thanks Tim.

@ryantam626
Copy link
Contributor Author

@ryantam626 ryantam626 commented Mar 28, 2020

Ah I was gonna work on this once weekend has arrived, but I see there is already a PR for this, I will leave this in @tgeorgeux 's good hands then 🙏

@lock lock bot added the status:resolved-locked label May 6, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:notebook pkg:ui-components status:resolved-locked tag:CSS tag:Design System CSS tag:Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants