-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Nav enhancements #54
base: master
Are you sure you want to change the base?
Nav enhancements #54
Changes from all commits
47d34c5
2970dbd
eee9c31
cc056f4
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 |
---|---|---|
|
@@ -4,6 +4,7 @@ import {guard, replaceFuzzyDate} from '../fuzzy_date' | |
import {createDemo} from '../create-block-demo' | ||
import {updateShortcuts} from '../shortcuts' | ||
import {Roam} from '../../roam/roam' | ||
import { getFirstTopLevelBlock, getActiveEditElement } from '../../utils/dom' | ||
|
||
/** | ||
* Be cautious to reference functions on the objects via anonymous functions (e.g. see Roam.deleteBlock) | ||
|
@@ -20,6 +21,47 @@ const dispatchMap = new Map([ | |
|
||
browser.runtime.onMessage.addListener(command => dispatchMap.get(command)?.()) | ||
|
||
const enhanceNavigation = (event: KeyboardEvent) => { | ||
const isEditingTitle = event.target?.parentElement instanceof HTMLHeadingElement | ||
const isNoBlockActive = !Roam.getActiveRoamNode() | ||
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.
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 started out as an iOS dev, where there's a standard naming convention of phrasing boolean names with "is", as in, "Is no block active?" However, I can see that it's just causing confusion here, so I'll avoid it in future, and find something better in this case, should this PR remain open. 🙃 |
||
const isTopBlockActive = getActiveEditElement() === getFirstTopLevelBlock() | ||
const isInSearch = event.target instanceof HTMLInputElement | ||
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. this seems like an unreliable way to check for search. But I suppose you probably don't want to mess with any input element in general. So probably rename variable to reflect that 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. Good point. |
||
|
||
// Don't mess with arrow nav in search results | ||
if (isInSearch) return; | ||
|
||
switch (event.key) { | ||
case 'Enter': | ||
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. as discussed in previous PR, the Enter is probably redundant 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. Not sure what you mean here. In the other PR, I had 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. no, I meant this whole shortcut overall, as the standard |
||
if (isEditingTitle || isNoBlockActive) { | ||
Roam.activateTopBlock() | ||
} | ||
break | ||
case 'ArrowUp': | ||
if (isNoBlockActive || isTopBlockActive) { | ||
if (event.getModifierState("Shift")) { | ||
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. this seems indicative of potentially larger surface area of impact (e.g. this is gonna break alt+up or cmd+up/etc in top block, maybe other things). 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 take your point, and am considering extending the modifier check to include the full set of modifiers listed here so that it's impossible for there to be a conflict with existing shortcuts. 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'm still not entirely sure. seems like a rather intimate set of things to override and things can go wrong... that said, I think it can still be ok to include it. assuming we have a way to disable it and do careful testing that we don't introduce unexpected behaviors. |
||
// Break to avoid interfering with block selection | ||
break | ||
} | ||
Roam.activateTitle() | ||
} | ||
break | ||
case 'ArrowDown': | ||
if (isEditingTitle || isNoBlockActive) { | ||
if (event.getModifierState("Shift")) { | ||
// Break to avoid interfering with block selection | ||
break | ||
} | ||
Roam.activateTopBlock() | ||
} | ||
break | ||
} | ||
} | ||
|
||
document.addEventListener('keydown', ev => { | ||
// When used with 'keyup', this function fires twice. 🤔 | ||
enhanceNavigation(ev) | ||
}) | ||
|
||
document.addEventListener('keyup', ev => { | ||
if (ev.key === guard) replaceFuzzyDate() | ||
}) |
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.
this deserves to be it's own "feature" now with the ability to enable/disable it
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.
Fair enough. I'll put it behind a feature toggle.