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

Fixes toolbar button for Restart Kernel and Run All #13939

Merged
merged 11 commits into from
Feb 10, 2023

Conversation

JasonWeill
Copy link
Contributor

@JasonWeill JasonWeill commented Feb 7, 2023

References

Fixes #11898.

Code changes

Falls back to default label/caption if all labels/captions of subcommands in a semanticCommand are empty strings

User-facing changes

Before the change, the ⏩ toolbar button had the caption " and " (with leading/trailing spaces):

before change

After the change, the "Restart the kernel and run all cells" notebook toolbar button has an appropriate caption:

after change

Other toolbar buttons also have captions in sentence case (only the first word is capitalized) with definite articles used ("Interrupt Kernel" becomes "Interrupt the kernel").

Backwards-incompatible changes

None.

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@krassowski krassowski added the bug label Feb 7, 2023
@krassowski krassowski added this to the 4.0.0 milestone Feb 7, 2023
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be able to add a unit test to

describe('createSemanticCommand', () => {
for this one, please?

@JasonWeill
Copy link
Contributor Author

JasonWeill commented Feb 8, 2023

Upon further inspection, it looks like caption is always considered to be "" for the subcommands of a semanticCommand, even when I modified those commands to have a caption of their own. This seems to be the root cause of this issue.

(Edit: This is not correct; see comment below from Feb 8)

@JasonWeill
Copy link
Contributor Author

please update galata snapshots

@JasonWeill
Copy link
Contributor Author

JasonWeill commented Feb 8, 2023

This PR now fixes all toolbar buttons to have consistent casing by:

  1. Declaring caption for all commands used in the notebook toolbar, with sentence casing and definite articles used. For example, if label is "Interrupt Kernel", caption is "Interrupt the kernel".
  2. Using the notebook, not the runmenu, command for "Restart and run all". The semanticCommand logic would have capitalized the semantic command as "Restart the kernel and Run all cells" because each command starts with a capital letter. In the run menu, where the Title Case label is used, this is fine.

The semanticCommand logic is largely reverted in my changes today, although one change (to use label or caption for a single-command semanticCommand's label or caption) remains.

@JasonWeill JasonWeill marked this pull request as draft February 8, 2023 23:54
@JasonWeill JasonWeill marked this pull request as ready for review February 9, 2023 00:31
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JasonWeill

@fcollonval fcollonval merged commit 63f3280 into jupyterlab:master Feb 10, 2023
@krassowski
Copy link
Member

krassowski commented Feb 12, 2023

Since this was merged I saw the icon was gone but instead the text was shown:

Screenshot from 2023-02-12 12-54-45

This was because I had customised my buttons in toolbar (well I did not, but when you go to Notebook Panel in settings it saves the default to user settings) and the old runmenu:restart-and-run-all remains for me rather than getting the now-default.

To fix this manually go into Settings → Notebook Panel → find item with runmenu:restart-and-run-all and replace it with notebook:restart-run-all.

I think that this will affect everyone who opened settings Settings → Notebook Panel in earlier version as this wrongly triggers an update.

I am not sure if want to take any action here, just flagging this.

@JasonWeill
Copy link
Contributor Author

@krassowski I also saw this and I didn't realize that this was the cause. Is there something that I can do to automate this process on upgrade?

@krassowski
Copy link
Member

I guess we could consider shipping a migration script, but I think we don't attach application version to the settings which will makes it non trivial. Let's continue the discussion in #13972

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fast forward button in notebook toolbar has tooltip " and "
3 participants