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

Implement Undo & Redo keyboard shortcuts #382

Merged
merged 8 commits into from
Jul 30, 2024

Conversation

arjxn-py
Copy link
Member

@arjxn-py arjxn-py commented Jul 20, 2024

This is a rough try of me in which I tried to introduce simple keybindings for undo & redo i.e.

  • Ctrl + z for undo
  • Ctrl + y for redo

Summary of What I Did:

  • Created keybindings.json in which I have defined keybindings for undo and redo.
  • Loaded Keybindings inside Extension:
    • Modified the addCommands() function to load the keybindings.
    • Used a helper function loadKeybindings to register the keybindings with the command registry.

Copy link
Contributor

Binder 👈 Launch a Binder on branch arjxn-py/JupyterCAD/undo-redo-keybindings

packages/base/tsconfig.json Outdated Show resolved Hide resolved
packages/base/src/commands.ts Outdated Show resolved Hide resolved
packages/base/src/commands.ts Outdated Show resolved Hide resolved
@arjxn-py arjxn-py mentioned this pull request Jul 23, 2024
@arjxn-py
Copy link
Member Author

arjxn-py commented Jul 26, 2024

I studied QuantStack/jupytergis#58, i limited the selector to be the jupytercad-panel for undo & redo. Need one opinion- having keybindings.json is better or defining those keybindings inside the script only?

Edit: I personally feel that having a keybindings.json file is good because it is more modular.

@martinRenou
Copy link
Member

Edit: I personally feel that having a keybindings.json file is good because it is more modular.

Yep I think that looks good 👍🏽 Thanks

It looks like your PR is now conflicting with the main branch, due to the merge of the other PR

@arjxn-py
Copy link
Member Author

Resolving this branch asap.

packages/base/src/commands.ts Outdated Show resolved Hide resolved
packages/base/src/commands.ts Outdated Show resolved Hide resolved
@arjxn-py arjxn-py requested a review from trungleduc July 30, 2024 07:39
packages/base/src/keybindings.json Outdated Show resolved Hide resolved
Copy link
Member Author

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

If I define body as the selector, these keybindings enables for whole app.
It is fine in that case if we don't want to have key bindings irrespective of which component is focused.

packages/base/src/keybindings.json Outdated Show resolved Hide resolved
packages/base/src/keybindings.json Outdated Show resolved Hide resolved
@arjxn-py arjxn-py requested a review from martinRenou July 30, 2024 10:43
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Neat! Thanks a lot

@martinRenou martinRenou merged commit bbccb3d into jupytercad:main Jul 30, 2024
9 checks passed
@arjxn-py arjxn-py deleted the undo-redo-keybindings branch July 30, 2024 12:32
@SylvainCorlay
Copy link
Member

Arg. It seems the redo keyboard shortcut does not work.

@arjxn-py
Copy link
Member Author

arjxn-py commented Aug 2, 2024

Hi @SylvainCorlay, I just tested these keyboard shortcuts locally and on https://jupytercad.github.io/JupyterCAD/ as well and it seems to be working as expected for me on both the places.

Screen.Recording.2024-08-02.232030.mp4

However on a sidenote, I'd like to mention that we had an issue with the redo function in the past, which has been fixed recently by #385, thanks to @trungleduc. So in case you're trying this out locally, I'd once suggest to make sure that the code is properly synced to upstream. But in case the issue still persists, please let us know about the same. Thanks a lot for following up on this, I'd be more than happy to assist further.

@SylvainCorlay
Copy link
Member

Thanks for looking into it. The issue may be specific to the JupyterLite deployment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants