-
Notifications
You must be signed in to change notification settings - Fork 51.2k
[Devtools] Navigating commits performance panel hotkey #35238
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
Changes from 5 commits
557c800
6a7dc2f
99f12da
3ac3483
d53d7a5
dc73de4
00fe07d
f3cacf4
7d0407c
cee1159
075762e
694abec
b0a6257
922fb23
ebb9e16
e4247bc
884c16b
466f0d9
9ec729c
c661f24
5c84920
aa0b246
e719902
5b3d5b2
8dbeef0
69cda16
46ab188
0bd3009
d910eb6
c790fde
81751ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,8 @@ function Profiler(_: {}) { | |
| supportsProfiling, | ||
| startProfiling, | ||
| stopProfiling, | ||
| selectPrevCommitIndex, | ||
| selectNextCommitIndex, | ||
| } = useContext(ProfilerContext); | ||
|
|
||
| const {file: timelineTraceEventData, searchInputContainerRef} = | ||
|
|
@@ -63,9 +65,9 @@ function Profiler(_: {}) { | |
|
|
||
| const isLegacyProfilerSelected = selectedTabID !== 'timeline'; | ||
|
|
||
| // Cmd+E to start/stop profiler recording | ||
| const handleKeyDown = useEffectEvent((event: KeyboardEvent) => { | ||
| const correctModifier = isMac ? event.metaKey : event.ctrlKey; | ||
| // Cmd+E to start/stop profiler recording | ||
| if (correctModifier && event.key === 'e') { | ||
| if (isProfiling) { | ||
| stopProfiling(); | ||
|
|
@@ -74,6 +76,21 @@ function Profiler(_: {}) { | |
| } | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| } else if ( | ||
| isLegacyProfilerSelected && | ||
| didRecordCommits && | ||
| selectedCommitIndex !== null | ||
| ) { | ||
| // Left/Right to navigate commits | ||
| if (event.key === 'ArrowLeft' || event.key === 'ArrowRight') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably check for Cmd key as well. I think we should keep just arrow navigations for changes between nodes in tree view (outside of the scope for this PR), and
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree it's nicer to have them unique but the arrow navigation for the tree view nodes only works when one has just been selected and when that has happened even having them as different shortcuts you can't use a shortcut to navigate between commits. I'm not convinced how much the tab navigation one will be used if your mouse has already selected a tab in the area there just before, and therefore it could be nicer to leave the shortcut as arrow key for simplicity
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it would confuse people they are switching tabs when they expect to switch commits if they don't realise they need to select it
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I am following. We don't have keyboard navigation for the flamegraph in Profiler tree, right? But if we would add one, it would probably be done by using only arrow keys. If so, then it would conflict with the navigation between commits. That's my current thinking, what is the alternative? |
||
| if (event.key === 'ArrowLeft') { | ||
| selectPrevCommitIndex(); | ||
| } else { | ||
| selectNextCommitIndex(); | ||
| } | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.