Skip to content

fix: resolve conflicting shortcut key assignments in Combokeys #875

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

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

cheton
Copy link
Collaborator

@cheton cheton commented Oct 2, 2024

The wiki page has been updated as followed:
https://github.com/cncjs/cncjs/wiki/User-Guide#keyboard-shortcuts

  • ctrl + alt + command + ] or f - Jog Forward
  • ctrl + alt + command + [ or k - Jog Backward

PR Type

bug_fix


Description

  • Resolved a conflict in shortcut key assignments within the Combokeys library.
  • Updated the key combination for the Jog Backward command to prevent conflicts.

Changes walkthrough 📝

Relevant files
Bug fix
combokeys.js
Resolve conflicting shortcut key assignments in Combokeys

src/app/lib/combokeys.js

  • Changed the shortcut key assignment for the Jog Backward command.
  • Updated the key combination from 'ctrl+alt+command+b' to
    'ctrl+alt+command+k'.
  • +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    codesandbox bot commented Oct 2, 2024

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Conflict
    The new key combination 'ctrl+alt+command+k' might conflict with existing shortcuts. Verify that this combination is not used elsewhere in the application.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Use a constant for the key combination to improve maintainability

    Consider using a constant for the key combination to improve maintainability and
    reduce the risk of typos. This constant can be defined at the top of the file or in
    a separate configuration file.

    src/app/lib/combokeys.js [113]

    -keys: ['ctrl', 'alt', 'command', 'k'].join('+'),
    +keys: JOG_BACKWARD_KEYS,
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a constant for the key combination enhances maintainability by reducing the risk of typos and making updates easier. This suggestion is relevant and improves code quality.

    7
    Documentation
    Add a comment explaining the reason for changing the key combination

    Consider adding a comment explaining why the key combination was changed from 'b' to
    'k'. This will help future developers understand the reasoning behind this specific
    key assignment.

    src/app/lib/combokeys.js [112-114]

     { // Jog Backward (Alias)
    +  // Changed from 'b' to 'k' to avoid conflict with [reason for change]
       keys: ['ctrl', 'alt', 'command', 'k'].join('+'),
       cmd: 'JOG',
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a comment to explain the change in key combination aids in documentation and helps future developers understand the rationale behind the change. This suggestion is useful for maintaining clear code documentation.

    6

    💡 Need additional feedback ? start a PR chat

    @cheton cheton requested a review from emcniece October 2, 2024 07:18
    @cheton cheton merged commit a2c5649 into master Oct 2, 2024
    4 checks passed
    @cheton cheton deleted the cncjs-873 branch October 2, 2024 13:07
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    conflicting shortcut key assignment in combokeys.js
    1 participant