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 customizable shortcuts panel #1636

Merged
merged 122 commits into from
Sep 17, 2024
Merged

Conversation

ollimeier
Copy link
Collaborator

@ollimeier ollimeier commented Sep 4, 2024

This fixes #1594, fixes #1589, fixes #721, and fixes #1642.

Known issues:

  • event.preventDefault() does not work for default shortcuts like cmd+N (event.stopImmediatePropagation() does not work either)
    -> some default shortcuts cannot be overwritten, please see: bugs.chromium
    -> Good comment related to this issue

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

This isn't a proper review yet, I just noticed some small things.

src/fontra/client/lang/en.json Outdated Show resolved Hide resolved
src/fontra/views/applicationsettings/panel-base.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

Two more small things.

src/fontra/views/applicationsettings/panel-shortcuts.js Outdated Show resolved Hide resolved
src/fontra/views/applicationsettings/panel-shortcuts.js Outdated Show resolved Hide resolved
ollimeier and others added 2 commits September 5, 2024 09:19
Co-authored-by: Just van Rossum <justvanrossum@gmail.com>
Co-authored-by: Just van Rossum <justvanrossum@gmail.com>
@justvanrossum
Copy link
Collaborator

I noticed that if you assign a shortcut to the ellipse tool, the tool will switch, but the icon doesn't. This is technically unrelated to this PR, but perhaps it stll makes sense to fix it here.

Co-authored-by: Just van Rossum <justvanrossum@gmail.com>
@ollimeier
Copy link
Collaborator Author

I noticed that if you assign a shortcut to the ellipse tool, the tool will switch, but the icon doesn't. This is technically unrelated to this PR, but perhaps it stll makes sense to fix it here.

I agree. I fixed it here already: https://github.com/googlefonts/fontra/pull/1604/files
It's related to collapseSubTools.

@justvanrossum
Copy link
Collaborator

I fixed it here already: https://github.com/googlefonts/fontra/pull/1604/files

Ah, but that PR got merged into a branch which is now abandoned, right?

@ollimeier
Copy link
Collaborator Author

I fixed it here already: https://github.com/googlefonts/fontra/pull/1604/files

Ah, but that PR got merged into a branch which is now abandoned, right?

yes.

@ollimeier
Copy link
Collaborator Author

We have a layout issue with long labels:

image I don't think we can use the content size, as this will break alignment across sections. But:
  • The label could wrap
  • We could make the label column wider

(I think we should do both)

Thanks for the feedback. done.

@justvanrossum justvanrossum merged commit bc381ef into main Sep 17, 2024
3 checks passed
@justvanrossum justvanrossum deleted the issue-1594-custom-shortcuts branch September 17, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants