Add dashboard keyboard shortcuts - #9878
rohithreddykota wants to merge 1 commit into
Conversation
AdityaHegde
left a comment
AdityaHegde
left a comment
There was a problem hiding this comment.
Code wise this looks good. The only thing missing is localisation and text is hardcoded in english.
Feature wise, do we need more keyboard shortcuts? Feels wierd to be able to open the filter dropdown for example and arrow keys don't work.
nishantmonu51
added
Type:Feature
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
A couple of smaller things that did not warrant their own thread:
- The help list is unconditional, but several rows are dead in common views:
,has no target in time-dimension-detail or pivot (MetricsTimeSeriesCharts.svelte:315-334only renders the measure picker in the{:else}branch),.has no target outside the leaderboard branch (Dashboard.svelte:271-283), andChartInteractions.svelte:65only handlesewhen$explainEnabled. Hiding rows whose target is absent would keep the dialog honest. - Nine of the thirteen
DashboardShortcutActionvalues exist only to key the help list, soperformDashboardShortcut(DashboardShortcutAction.Zoom)type-checks but always returnsfalse. A plainid: stringonDashboardShortcutwould express that split more honestly.
| function handleKeydown(event: KeyboardEvent) { | ||
| const action = getDashboardShortcutAction(event); | ||
| if (!action || !performDashboardShortcut(action)) return; | ||
|
|
||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| } |
There was a problem hiding this comment.
handleKeydown never consults open, so the picker shortcuts stay live while the shortcuts dialog is up. Focus sits inside the body-portalled Dialog.Content, which is not an editable target, so /, , or . still resolves an action and performDashboardShortcut finds the trigger that is still mounted behind the overlay and clicks it. Both bits-ui triggers accept that synthetic click (DropdownMenuTriggerState.onclick only rejects e.detail !== 0, and PopoverTriggerState.onclick toggles on button === 0), so the dropdown opens behind the modal and the user is left with two overlapping surfaces to dismiss. The same holds for any other open modal, not just this one. Only ToggleHelp should be honoured while open is true.
| export const DASHBOARD_SHORTCUTS: DashboardShortcut[] = [ | ||
| { | ||
| action: DashboardShortcutAction.ToggleHelp, | ||
| keys: ["?"], | ||
| description: "Show/hide the keyboard-shortcuts menu", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.OpenFilter, | ||
| keys: ["/"], | ||
| description: "Open the Filter menu", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.OpenMetricPicker, | ||
| keys: [","], | ||
| description: "Open the metric picker", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.OpenDimensionPicker, | ||
| keys: ["."], | ||
| description: "Open the dimension picker", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.InspectCell, | ||
| keys: ["Space"], | ||
| description: "Show/hide the cell inspector", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.LockInspector, | ||
| keys: ["L"], | ||
| description: "Lock/unlock the cell inspector", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.ClearSelection, | ||
| keys: ["Esc"], | ||
| description: "Close the inspector or clear a chart selection", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.SelectAll, | ||
| keys: ["⌘/Ctrl", "A"], | ||
| description: "Select all dimension values", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.PanLeft, | ||
| keys: ["←"], | ||
| description: "Pan the chart backward", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.PanRight, | ||
| keys: ["→"], | ||
| description: "Pan the chart forward", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.Zoom, | ||
| keys: ["Z"], | ||
| description: "Zoom into the selected range", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.UndoZoom, | ||
| keys: ["⌘/Ctrl", "Z"], | ||
| description: "Undo chart zoom", | ||
| }, | ||
| { | ||
| action: DashboardShortcutAction.Explain, | ||
| keys: ["E"], | ||
| description: "Explain the selected range", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
@AdityaHegde already flagged the hardcoded copy; adding the specifics. All thirteen descriptions here, plus the dialog title, the description and the aria-label on the sr-only button in DashboardShortcuts.svelte:27,36-39, are literal English, while AddExpressionFilterButton.svelte already reads its copy from paraglide two lines below the hunk this PR adds (aria-label={m.dashboard_add_filter_button()}). scripts/i18n-guard.js only scans files that import the messages namespace or match MIGRATED_GLOBS, and these new files do neither, so CI will not flag it and it becomes silent migration debt. The keys need to be added to both web-common/src/lib/i18n/messages/en.json and es.json.
This branch has not been deployed
3 participants
Adds dashboard-scoped keyboard shortcuts and a discoverable help dialog.
?to show or hide the keyboard-shortcuts menu./,,, or.to open the filter, metric, or dimension picker.Tested with
npm test --workspace web-common -- src/features/dashboards/shortcuts/dashboard-shortcuts.spec.ts(9 tests passing).Checklist: