Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/ts/contentScripts/dispatcher/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -20,6 +21,47 @@ const dispatchMap = new Map([

browser.runtime.onMessage.addListener(command => dispatchMap.get(command)?.())

const enhanceNavigation = (event: KeyboardEvent) => {
Copy link
Member

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

Copy link
Author

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.

const isEditingTitle = event.target?.parentElement instanceof HTMLHeadingElement
const isNoBlockActive = !Roam.getActiveRoamNode()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notInRoamBlock?

Copy link
Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The 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':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed in previous PR, the Enter is probably redundant

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean here. In the other PR, I had const enter = 'Enter'. Here there's no intermediary storage, just the raw string. ¯_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, I meant this whole shortcut overall, as the standard cmd-enter exists

if (isEditingTitle || isNoBlockActive) {
Roam.activateTopBlock()
}
break
case 'ArrowUp':
if (isNoBlockActive || isTopBlockActive) {
if (event.getModifierState("Shift")) {
Copy link
Member

Choose a reason for hiding this comment

The 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).
Makes me hesitate about overriding things that are so fundamental. How about just adding a shortcut to edit the title?

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.
What about just scrolling the page with the keyboard? (I don't use it as I use vim mappings, but seems plausibly like something people use?)

// 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()
})
11 changes: 10 additions & 1 deletion src/ts/roam/roam.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {RoamNode, Selection} from './roam-node'
import {getActiveEditElement, getFirstTopLevelBlock, getInputEvent, getLastTopLevelBlock} from '../utils/dom'
import {Keyboard} from '../utils/keyboard'
import {Mouse} from '../utils/mouse'
import { getActiveEditElement, getFirstTopLevelBlock, getInputEvent, getLastTopLevelBlock, getTitleElement } from '../utils/dom'

export const Roam = {
save(roamNode: RoamNode) {
Expand Down Expand Up @@ -46,6 +46,15 @@ export const Roam = {
return Promise.reject("We're currently not inside roam block")
},

async activateTitle() {
await Mouse.leftClick(getTitleElement())
},

async activateTopBlock() {
await Mouse.leftClick(getFirstTopLevelBlock())
this.moveCursorToStart()
},

async activateBlock(element: HTMLElement) {
if (element.classList.contains('roam-block')) {
await Mouse.leftClick(element)
Expand Down
4 changes: 4 additions & 0 deletions src/ts/utils/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ export function getActiveEditElement(): ValueElement {
return element as ValueElement
}

export function getTitleElement() {
return document.querySelector('.rm-title-display') as HTMLElement
}

export function getTopLevelBlocks() {
return document.querySelector('.roam-article div .flex-v-box') as HTMLElement
}
Expand Down