-
-
Notifications
You must be signed in to change notification settings - Fork 362
feat(DateTimePicker): support Tab/Esc keyboard event #6848
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
Conversation
Reviewer's GuideThis PR enhances the DateTimePicker component by integrating keyboard support for Tab and Escape keys through EventHandler, updating popover show/hide logic, and ensuring proper cleanup of event listeners upon disposal. Sequence diagram for DateTimePicker Tab/Escape keyboard event handlingsequenceDiagram
participant User as actor User
participant Input as DateTimePicker Input
participant Popover as Popover
participant EventHandler as EventHandler
User->>Input: Press Tab (keydown)
Input->>Popover: isShown()
alt Popover is shown
Popover->>Popover: hide()
end
User->>Input: Press Escape (keyup)
Input->>Popover: hide()
Input->>Input: blur()
User->>Input: Press Tab (keyup)
Input->>Popover: show()
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds keyboard event support to the DateTimePicker component, specifically handling Tab and Escape key interactions to improve user experience and accessibility.
- Added Tab key support to show/hide the popover based on key event type
- Added Escape key support to hide the popover and blur the input field
- Implemented proper event handler cleanup in the dispose function
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| DateTimePicker.razor.js | Added keyboard event handling for Tab/Esc keys with proper event cleanup |
| BootstrapBlazor.csproj | Version bump from beta05 to beta06 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The Tab key logic currently hides the popover on keydown then shows it again on keyup, which might cause a flicker or unexpected behavior—consider consolidating this into a single event or adjusting the timing to ensure a smooth UX.
- In dispose(), using EventHandler.off without a specific handler reference will remove all keydown/keyup listeners on the input; you should store each callback and pass it to off() to avoid unregistering unrelated handlers.
- You may need to call e.preventDefault() or check focus state when handling Tab to avoid interfering with native tab navigation and ensure the popover only toggles when appropriate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Tab key logic currently hides the popover on keydown then shows it again on keyup, which might cause a flicker or unexpected behavior—consider consolidating this into a single event or adjusting the timing to ensure a smooth UX.
- In dispose(), using EventHandler.off without a specific handler reference will remove all keydown/keyup listeners on the input; you should store each callback and pass it to off() to avoid unregistering unrelated handlers.
- You may need to call e.preventDefault() or check focus state when handling Tab to avoid interfering with native tab navigation and ensure the popover only toggles when appropriate.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.js:22` </location>
<code_context>
}
}
});
+ const input = el.querySelector('.datetime-picker-input');
const dateTimePicker = {
- el,
</code_context>
<issue_to_address>
**issue:** Consider handling the case where input is not found.
Without a check for a missing input, attaching event handlers may fail if the selector does not match. Adding error handling will make the code more robust to DOM changes.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.js:29` </location>
<code_context>
- Data.set(id, dateTimePicker)
+ Data.set(id, dateTimePicker);
+
+ EventHandler.on(input, 'keydown', e => {
+ if (e.key === 'Tab' && popover.isShown()) {
+ popover.hide();
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing two EventHandler listeners with a single native keydown handler to simplify event management.
Here’s one way to collapse both listeners into a single native handler (no EventHandler wrapper) and still be able to clean it up on dispose:
```js
// init(...)
const input = el.querySelector('.datetime-picker-input');
function onKeydown(e) {
switch (e.key) {
case 'Escape':
popover.hide();
input.blur();
break;
case 'Tab':
popover[popover.isShown() ? 'hide' : 'show']();
break;
}
}
input.addEventListener('keydown', onKeydown);
Data.set(id, { input, popover, onKeydown });
```
```js
// dispose(id)
const data = Data.get(id);
if (data) {
const { input, popover, onKeydown } = data;
input.removeEventListener('keydown', onKeydown);
Popover.dispose(popover);
Data.remove(id);
}
```
This:
- Uses a single `keydown` listener for both `Tab` and `Escape`
- Eliminates the extra indirection of `EventHandler`
- Stores the raw handler so you can clean it up properly in `dispose` without extra branching.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6848 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 739 739
Lines 31755 31755
Branches 4466 4466
=========================================
Hits 31755 31755
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #6847
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add keyboard handling for Tab and Escape keys in DateTimePicker to improve popover interaction and ensure event listeners are cleaned up on disposal
New Features:
Enhancements: