feat: add Enter key support with hint in condition dialog#2315
feat: add Enter key support with hint in condition dialog#2315
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
.enter-hintclass setsalign-itemsandgapbut never specifies a flex display, so these properties have no effect; consider addingdisplay: inline-flex(or similar) to get the desired layout. - Binding
(keydown.enter)on the entiremat-dialog-contentmay triggeradd()when Enter is pressed in other controls inside the dialog; consider attaching the handler directly to thelhsinput (or checking the event target) to avoid unintended submissions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `.enter-hint` class sets `align-items` and `gap` but never specifies a flex display, so these properties have no effect; consider adding `display: inline-flex` (or similar) to get the desired layout.
- Binding `(keydown.enter)` on the entire `mat-dialog-content` may trigger `add()` when Enter is pressed in other controls inside the dialog; consider attaching the handler directly to the `lhs` input (or checking the event target) to avoid unintended submissions.
## Individual Comments
### Comment 1
<location path="src/app/shared/modules/search-parameters-dialog/search-parameters-dialog.component.html" line_range="3" />
<code_context>
<h2 mat-dialog-title>Add Characteristic</h2>
<form [formGroup]="parametersForm" class="search-parameters-form">
- <mat-dialog-content>
+ <mat-dialog-content (keydown.enter)="add()">
<mat-form-field class="search-field">
<mat-label>Field</mat-label>
</code_context>
<issue_to_address>
**issue (bug_risk):** Handling Enter on the whole dialog content can trigger `add()` in unintended contexts (e.g. on buttons or other controls).
Binding `(keydown.enter)` on `mat-dialog-content` will call `add()` for any Enter press inside the dialog, including on buttons or future fields. Instead, bind Enter only on the input that should submit (e.g. `lhs` or the form via `(ngSubmit)`), and if needed use `stopPropagation` / `preventDefault` there to avoid duplicate handling.
</issue_to_address>
### Comment 2
<location path="src/app/shared/modules/search-parameters-dialog/search-parameters-dialog.component.html" line_range="108-109" />
<code_context>
<mat-dialog-actions [attr.align]="'end'">
- <button mat-button (click)="cancel()">Cancel</button>
- <button mat-button type="submit" (click)="add()" color="primary">
+ <button mat-button type="button" (click)="cancel()">Cancel</button>
+ <button mat-button type="button" (click)="add()" color="primary">
Apply
</button>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Changing the buttons to `type="button"` removes native form submission behavior, which could be handled more cleanly at the form level.
With both buttons as `type="button"`, the form loses native submit behavior and relies on a container-level `keydown.enter` handler. Consider wiring `(ngSubmit)="add()"` on the form and keeping the Apply button as `type="submit"`. This centralizes submit logic, improves accessibility/keyboard support, and may let you remove the broad keydown listener.
Suggested implementation:
```
</mat-dialog-content>
<form (ngSubmit)="add()">
<mat-dialog-actions [attr.align]="'end'">
<button mat-button type="button" (click)="cancel()">Cancel</button>
<button mat-button type="submit" color="primary">
Apply
</button>
</mat-dialog-actions>
</form>
```
1. If there is a `keydown.enter` or similar global/container-level handler in the corresponding TypeScript file (e.g., `search-parameters-dialog.component.ts`) that was previously used to trigger `add()`, it can likely be removed because the form submit will now handle Enter key presses on any input within the form.
2. Ensure that there is no nested `<form>` higher up in the template (wrapping this dialog), as nested forms are invalid HTML. If there is an outer form, instead add `(ngSubmit)="add()"` and `type="submit"` there, and do not introduce another `<form>` here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <h2 mat-dialog-title>Add Characteristic</h2> | ||
| <form [formGroup]="parametersForm" class="search-parameters-form"> | ||
| <mat-dialog-content> | ||
| <mat-dialog-content (keydown.enter)="add()"> |
There was a problem hiding this comment.
issue (bug_risk): Handling Enter on the whole dialog content can trigger add() in unintended contexts (e.g. on buttons or other controls).
Binding (keydown.enter) on mat-dialog-content will call add() for any Enter press inside the dialog, including on buttons or future fields. Instead, bind Enter only on the input that should submit (e.g. lhs or the form via (ngSubmit)), and if needed use stopPropagation / preventDefault there to avoid duplicate handling.
| <button mat-button type="button" (click)="cancel()">Cancel</button> | ||
| <button mat-button type="button" (click)="add()" color="primary"> |
There was a problem hiding this comment.
suggestion (bug_risk): Changing the buttons to type="button" removes native form submission behavior, which could be handled more cleanly at the form level.
With both buttons as type="button", the form loses native submit behavior and relies on a container-level keydown.enter handler. Consider wiring (ngSubmit)="add()" on the form and keeping the Apply button as type="submit". This centralizes submit logic, improves accessibility/keyboard support, and may let you remove the broad keydown listener.
Suggested implementation:
</mat-dialog-content>
<form (ngSubmit)="add()">
<mat-dialog-actions [attr.align]="'end'">
<button mat-button type="button" (click)="cancel()">Cancel</button>
<button mat-button type="submit" color="primary">
Apply
</button>
</mat-dialog-actions>
</form>
- If there is a
keydown.enteror similar global/container-level handler in the corresponding TypeScript file (e.g.,search-parameters-dialog.component.ts) that was previously used to triggeradd(), it can likely be removed because the form submit will now handle Enter key presses on any input within the form. - Ensure that there is no nested
<form>higher up in the template (wrapping this dialog), as nested forms are invalid HTML. If there is an outer form, instead add(ngSubmit)="add()"andtype="submit"there, and do not introduce another<form>here.
Description
This PR updates the condition dialog in Dataset and Sample pages so users can type a metadata key and press Enter to add a condition, and it adds a hint too
Motivation
Background on use case, changes needed
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Add keyboard support and UI hint for applying conditions in the search parameters dialog for datasets and samples.
New Features:
Enhancements:
Tests: