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 shortcuts #612

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Implement shortcuts #612

wants to merge 3 commits into from

Conversation

guergana
Copy link
Collaborator

@guergana guergana commented Oct 24, 2024

TODO : test for Mac

@guergana guergana marked this pull request as draft October 24, 2024 11:43
Copy link

cloudflare-workers-and-pages bot commented Oct 24, 2024

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 219b01f
Status: ✅  Deploy successful!
Preview URL: https://edaee196.opendataeditor.pages.dev
Branch Preview URL: https://606-shortcuts.opendataeditor.pages.dev

View logs

@romicolman
Copy link
Collaborator

Let's implement the rest and omit the Ctrl+Q for now

@guergana
Copy link
Collaborator Author

Let's implement the rest and omit the Ctrl+Q for now

Hello @romicolman I just discovered many shortcuts that were already implemented in the app before this change. I think this is not desired. For example: ctrl + k brings up the Publish dialog.

You can see the shortcuts that are already implemented in this file: https://github.com/okfn/opendataeditor/blob/main/client/components/Parts/Bars/Action.tsx#L40

and this https://github.com/okfn/opendataeditor/blob/main/client/components/Parts/Bars/Menu.tsx#L56

There is one here too: https://github.com/okfn/opendataeditor/blob/main/client/components/Parts/Chips/Create.tsx#L13

I guess we need to remove these ones as well as part of this task?

@guergana guergana marked this pull request as ready for review October 28, 2024 18:07
@romicolman
Copy link
Collaborator

Hi @guergana! Thank you! Let's remove all and just keep the ones that are in the ticket, except from Ctrl + Q

Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

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

Hi! I have just tested the implementation. Apart from ENTER, ESC, none of the shortcuts work on Mac. Can we check if we need to define different parameters for this OS?

@romicolman
Copy link
Collaborator

@guergana just in case this is useful, here are some Mac shortcuts:https://support.apple.com/en-us/102650

Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

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

Changes on Mac are not working (I'll create a new ticket to address this) but please, implement this so we can quickly merge this to the main code for Windows and Linux users.

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.

Implement shortcuts
3 participants