Nav enhancements #54

clozach · 2020-05-14T19:51:16Z

As mentioned here, I decided to roll feedback from @Stvad into a new branch with additional navigation improvements in the form of arrow key navigation.

These small tweaks make my own experience in Roam feel much less stilted, and I hope you feel the same after trying them out!

Feedback welcome, esp. regarding…

Do you see/experience any bugs here?
Is there anything about the code structure, naming, and/or organization you'd like to see changed before considering a merge?

…no regular nodes are currently active OR …the title is active Problem: When no nodes are selected, or the title is currently being edited, hitting `Enter` is a no-op. Solution: Hitting `Enter` in these two cases now puts the cursor in a new node at the top of the page.

Problem: The arrow keys stop doing anything useful when either no nodes are active or the cursor is in the title or top node Solution: - When no nodes are active, Up Arrow (↑) activates the title - From the top node, Up Arrow (↑) activates the title - From the title, Down Arrow (↓) activates the top node - When no nodes are active, Up Arrow (↓) activates the top node

When the top node starts with a tag, `activateTopBlock` places the cursor _after_ the tag by default. This tweak corrects that strange behavior. # Steps to reproduce I have a page where the first node looks like this: #character #father #grandfather #[[Book(0)]] Before this commit, hitting Enter from within the title would place the cursor here… #character↓ #father #grandfather #[[Book(0)]] …instead of here… ↓#character #father #grandfather #[[Book(0)]]

…navigation

Stvad · 2020-05-16T01:53:31Z

src/ts/contentScripts/dispatcher/index.ts

+    if (isInSearch) return;
+
+    switch (event.key) {
+        case 'Enter':


as discussed in previous PR, the Enter is probably redundant

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. ¯_(ツ)_/¯

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

Stvad · 2020-05-16T01:54:24Z

src/ts/contentScripts/dispatcher/index.ts

@@ -20,6 +21,47 @@ const dispatchMap = new Map([

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

+const enhanceNavigation = (event: KeyboardEvent) => {


this deserves to be it's own "feature" now with the ability to enable/disable it

Fair enough. I'll put it behind a feature toggle.

Stvad · 2020-05-16T01:55:15Z

src/ts/contentScripts/dispatcher/index.ts

@@ -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()


notInRoamBlock?

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

Stvad · 2020-05-16T01:58:48Z

src/ts/contentScripts/dispatcher/index.ts

+    const isEditingTitle = event.target?.parentElement instanceof HTMLHeadingElement
+    const isNoBlockActive = !Roam.getActiveRoamNode()
+    const isTopBlockActive = getActiveEditElement() === getFirstTopLevelBlock()
+    const isInSearch = event.target instanceof HTMLInputElement


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

Good point.

Stvad · 2020-05-16T02:04:11Z

src/ts/contentScripts/dispatcher/index.ts

+            break
+        case 'ArrowUp':
+            if (isNoBlockActive || isTopBlockActive) {
+                if (event.getModifierState("Shift")) {


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?

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.

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?)

clozach

Thanks for the feedback, @Stvad! I hope to have your issues completely addressed in the next day or two.

clozach · 2020-05-17T22:01:57Z

src/ts/contentScripts/dispatcher/index.ts

@@ -20,6 +21,47 @@ const dispatchMap = new Map([

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

+const enhanceNavigation = (event: KeyboardEvent) => {


Fair enough. I'll put it behind a feature toggle.

clozach · 2020-05-17T22:04:09Z

src/ts/contentScripts/dispatcher/index.ts

@@ -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()


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

clozach · 2020-05-17T22:06:59Z

src/ts/contentScripts/dispatcher/index.ts

+    const isEditingTitle = event.target?.parentElement instanceof HTMLHeadingElement
+    const isNoBlockActive = !Roam.getActiveRoamNode()
+    const isTopBlockActive = getActiveEditElement() === getFirstTopLevelBlock()
+    const isInSearch = event.target instanceof HTMLInputElement


Good point.

clozach · 2020-05-17T22:07:19Z

src/ts/contentScripts/dispatcher/index.ts

+    if (isInSearch) return;
+
+    switch (event.key) {
+        case 'Enter':


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. ¯_(ツ)_/¯

clozach · 2020-05-17T22:28:45Z

src/ts/contentScripts/dispatcher/index.ts

+            break
+        case 'ArrowUp':
+            if (isNoBlockActive || isTopBlockActive) {
+                if (event.getModifierState("Shift")) {


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.

clozach added 4 commits May 13, 2020 16:53

Prevent keyboard navigation enhancements from breaking Search result …

cc056f4

…navigation

clozach mentioned this pull request May 14, 2020

Get from title to top block wiithout mousing #50

Closed

Stvad reviewed May 16, 2020

View reviewed changes

clozach commented May 17, 2020

View reviewed changes

tntmarket mentioned this pull request May 18, 2020

Many vim-like shortcuts for navigating and manipulating blocks #63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nav enhancements #54

Nav enhancements #54

clozach commented May 14, 2020

Stvad May 16, 2020

clozach May 17, 2020

Stvad May 18, 2020

Stvad May 16, 2020

clozach May 17, 2020

Stvad May 16, 2020

clozach May 17, 2020

Stvad May 16, 2020

clozach May 17, 2020

Stvad May 16, 2020

clozach May 17, 2020

Stvad May 18, 2020

clozach left a comment

clozach May 17, 2020

clozach May 17, 2020

clozach May 17, 2020

clozach May 17, 2020

clozach May 17, 2020

		@@ -20,6 +21,47 @@ const dispatchMap = new Map([

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

		const enhanceNavigation = (event: KeyboardEvent) => {

Nav enhancements #54

Are you sure you want to change the base?

Nav enhancements #54

Conversation

clozach commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clozach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment