check if in popup before triggering shortcut #2789
Conversation
Reviewer's Guide by SourceryThis pull request introduces changes to prevent keyboard shortcuts from being triggered when the focus is within a popup dialog. It also modifies the dialog component to emit events on form submission, improving event handling. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @imolorhe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses an issue where shortcuts were being triggered even when a popup/modal was active. To fix this, I've added a check to ensure that the active element is not within an nz-modal-container before allowing the shortcut to execute. I've also added a hidden submit button to the dialog component to allow submitting the form when pressing enter.
Highlights
- Shortcut Handling: Added a check to prevent shortcuts from triggering when a modal/popup is active. This is done by checking if the
document.activeElementis within annz-modal-container. - Dialog Submission: Added a hidden submit button within the dialog component's form to enable form submission when the user presses the Enter key.
- Keybinder Service: Modified the
bindShortcutfunction inKeybinderServiceto accept anallowInPopupsparameter, defaulting tofalse. This parameter controls whether the shortcut should be active when a popup is open.
Changelog
- packages/altair-app/src/app/modules/altair/components/dialog/dialog.component.html
- Wrapped the
ng-contentwithin a<form>tag and added a hidden submit button. This allows the dialog to be submitted when pressing enter.
- Wrapped the
- packages/altair-app/src/app/modules/altair/components/dialog/dialog.component.ts
- Added an
onSubmitmethod that emits thetoggleDialogandsaveChangeevents, similar to theonClickSavemethod.
- Added an
- packages/altair-app/src/app/modules/altair/services/keybinder/keybinder.service.ts
- Added a check in the shortcut handler to prevent execution if
allowInPopupsis false and the current element is within a popup. - Added a private
isInPopupmethod to check if the active element is within annz-modal-container. - Modified the
bindShortcutfunction to accept anallowInPopupsparameter. - Set
allowInPopupstotruefor 'Toggle Variable Pane' and 'Toggle Header Pane' shortcuts.
- Added a check in the shortcut handler to prevent execution if
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A shortcut pressed, a function called,
But wait, a popup stands so tall.
No action taken, the popup's the key,
To keep the workflow smooth and free.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a fix to prevent shortcuts from being triggered when a dialog/popup is open. This is achieved by adding a check to see if the active element is within an nz-modal-container before executing the shortcut callback. Additionally, the PR adds a parameter to the bindShortcut function to allow specifying whether a shortcut should be allowed in popups. Overall, the changes seem reasonable and address the issue effectively.
Summary of Findings
- Dialog Submission: The dialog component now includes a form that wraps the content. This change might have unintended consequences if the content already contains a form or relies on specific event handling. Ensure that this change doesn't break existing dialog implementations.
- Shortcut Binding: The
bindShortcutfunction now accepts anallowInPopupsparameter. This is a good addition, but it's important to ensure that all existing shortcut bindings are reviewed to determine the appropriate value for this parameter. Some shortcuts might be intended to work in popups, while others should not. - Popup Detection: The
isInPopupfunction relies onnz-modal-containerto detect if the active element is within a popup. This might not be reliable if a different modal implementation is used. Consider making this more generic or configurable.
Merge Readiness
The pull request addresses a valid issue and the changes seem to be well-structured. However, before merging, it's crucial to address the potential issues identified in the findings summary, particularly regarding the dialog submission and shortcut bindings. I am unable to directly approve the pull request, and users should have others review and approve this code before merging. Given the medium severity comments, I recommend addressing these before merging.
| <form (ngSubmit)="onSubmit($event)"> | ||
| <ng-content></ng-content> | ||
| <button type="submit" style="display: none"></button> | ||
| </form> |
There was a problem hiding this comment.
Wrapping the content in a <form> element might interfere with existing form elements within the dialog content. Consider if this is the best approach, or if there's a way to prevent potential conflicts. For example, could you add a class to the form and target that class in your CSS to avoid style conflicts?
| <form (ngSubmit)="onSubmit($event)"> | |
| <ng-content></ng-content> | |
| <button type="submit" style="display: none"></button> | |
| </form> | |
| <div class="dialog-form"> | |
| <ng-content></ng-content> | |
| <button type="submit" style="display: none"></button> | |
| </div> |
| description: string, | ||
| allowInPopups = false |
There was a problem hiding this comment.
The description parameter is now required. Consider providing a default value that indicates the description is missing, rather than forcing every call site to provide one. Also, the TODO comment is now gone, but it would be good to have a description for each shortcut.
| description: string, | |
| allowInPopups = false | |
| description: string = 'No description provided', |
|
|
||
| private isInPopup() { | ||
| const activeElement = document.activeElement as HTMLElement | null; | ||
| return !!activeElement?.closest('nz-modal-container'); |
There was a problem hiding this comment.
This function relies on the presence of nz-modal-container. If a different modal implementation is used, this check will fail. Consider making this more generic or configurable, perhaps by allowing a custom selector to be passed in.
| return !!activeElement?.closest('nz-modal-container'); | |
| return !!activeElement?.closest('nz-modal-container, .custom-modal-container'); |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @imolorhe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a unit test for the
isInPopupmethod to ensure it correctly identifies when the active element is within annz-modal-container. - It might be helpful to add a comment explaining why certain shortcuts should not be triggered within popups.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Visit the preview URL for this PR (updated for commit 7731146): https://altair-gql--pr2789-imolorhe-check-popup-69kc97y4.web.app (expires Wed, 12 Mar 2025 23:24:28 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
Fixes
Checks
yarn test-buildChanges proposed in this pull request:
Summary by Sourcery
Prevent shortcuts from being triggered when a popup is open. This change ensures that shortcuts are only triggered when the main application window is in focus, preventing unintended actions within popups. Additionally, this PR modifies the dialog component to emit events on save and submit.
Enhancements: