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

Correct keyboard shortcuts commands for headings #4430

Merged
merged 1 commit into from Apr 20, 2018

Conversation

@logstar
Copy link
Contributor

@logstar logstar commented Apr 20, 2018

The proposed changes were based on @jasongrout's suggestions in issue #4428.

In jupyterlab/packages/shortcuts-extension/schema/plugin.json, keyboard shortcut 1-6 are registered with commands change-to-cell-heading-X, whereas the commands in jupyterlab/packages/notebook-extension/src/index.ts are named change-cell-to-heading-X. As a result, the keyboard shortcuts are not working.

This PR attempts to fix this problem by changing the command names in jupyterlab/packages/shortcuts-extension/schema/plugin.json to the ones in jupyterlab/packages/notebook-extension/src/index.ts.

The proposed changes were based on @jasongrout's suggestions in issue jupyterlab#4428.

In `jupyterlab/packages/shortcuts-extension/schema/plugin.json`, keyboard shortcut `1-6` are registered with commands `change-`**`to-cell`**`-heading-X`, whereas the commands in `jupyterlab/packages/notebook-extension/src/index.ts` are named `change-`**`cell-to`**`-heading-X`. As a result, the keyboard shortcuts are not working. 

This PR attempts to fix this problem by changing the command names in `jupyterlab/packages/shortcuts-extension/schema/plugin.json` to the ones in `jupyterlab/packages/notebook-extension/src/index.ts`.
@@ -256,55 +256,55 @@
},
"type": "object"
},
"notebook:change-to-cell-heading-1": {
"notebook:change-cell-to-heading-1": {
Copy link
Contributor

@jasongrout jasongrout Apr 20, 2018

Can you leave the names of the shortcuts the same, and just change the "command" lines, so that if someone overrode the setting, it still is overridden? We'll change the name of the shortcuts in the 0.33 release, since it's a backwards-incompatible change.

Copy link
Contributor

@jasongrout jasongrout Apr 20, 2018

That way we can release this fix in a patch release to 0.32, rather than having to wait until 0.33.

Copy link
Contributor

@jasongrout jasongrout Apr 20, 2018

On second thought, we're already merging changes for 0.33, so I think this should just go in as-is. We can do a backport that is backwards compatible for the 0.32 series.

Copy link
Contributor Author

@logstar logstar Apr 20, 2018

Thank you for the suggestions and clarifications. Sorry that I did not fully understand your comments in the issue.

Working on it.

Copy link
Contributor Author

@logstar logstar Apr 20, 2018

Sorry that I am not familiar with formal version control systems.

I was wondering if there is anything else I could help on.

Thank you again for your time.

Copy link
Contributor

@jasongrout jasongrout Apr 20, 2018

I think we're good. I can make a similar change to the 0.32 branch once we have that set up so it can come out before 0.33. Sorry for the confusion.

Copy link
Contributor Author

@logstar logstar Apr 20, 2018

No worries at all. I was worried about causing you troubles.

Learned a lot. I found it is very necessary for me to review the basics of formal software project management, in order to better contribute on open source projects.

Thank you!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 20, 2018

Thanks! I made one comment inline with the code.

@jasongrout jasongrout added this to the Beta 2 Patch milestone Apr 20, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 20, 2018

Thanks!

@jasongrout jasongrout merged commit 8e0ed9f into jupyterlab:master Apr 20, 2018
2 checks passed
@jasongrout jasongrout removed this from the Beta 2 Patch milestone Apr 20, 2018
@jasongrout jasongrout added this to the Beta 3 milestone Apr 20, 2018
logstar added a commit to logstar/jupyterlab that referenced this issue Apr 20, 2018
Based on @jasongrout's suggestions in pull request jupyterlab#4430, the shortcut names for changing headings are changed back to `change-to-cell-heading-X` for compatibility concerns.

Quotes from @jasongrout's comment:

"Can you leave the names of the shortcuts the same, and just change the 'command' lines, so that if someone overrode the setting, it still is overridden? We'll change the name of the shortcuts in the 0.33 release, since it's a backwards-incompatible change."

"That way we can release this fix in a patch release to 0.32, rather than having to wait until 0.33."
jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Apr 24, 2018
jasongrout added a commit that referenced this issue Apr 25, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 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.

None yet

2 participants