migrate commands to command palette (Closes #389) #391

mdroidian · 2023-03-23T02:43:02Z

Let me know if I'm on the right track 👍 @dvargas92495

components PR: RoamJS/roamjs-components#3

mdroidian · 2023-03-23T02:47:58Z

Also, the docs seem to strongly recommend against setting a default shortcut.
Do we still want to go ahead with that?

dvargas92495

We should also make sure the removeCommand is added in roamjs-components

src/features/tutorials.tsx

src/index.ts

mdroidian · 2023-03-26T00:10:55Z

Struggling with Typescript definition and console error with the decorators.tsx: d4d78f7

Typescript error:

Type '{ settings: 
{ get: (k: string) => unknown; getAll: () => Record<string, unknown>; 
panel: { create: (c: PanelConfig) => void; }; 
set: (k: string, v: unknown) => Promise<void>; }; 
ui: { commandPalette?: { ...; }; }; }' 
is not assignable to type 'never'.ts(2322)

Console error:

Uncaught TypeError: parseRoamMarked is not a function
    at callback (decorators.tsx:482:27)
    at callback (createHashtagObserver.js:12:13)
    at Array.forEach (<anonymous>)
    at createHTMLObserver (createHTMLObserver.js:8:27)
    at createHashtagObserver (createHashtagObserver.js:5:94)
    at toggleFeature (decorators.tsx:473:119)
    at onClick (decorators.tsx:335:7)
...

dvargas92495 · 2023-03-26T02:29:52Z

src/features/decorators.tsx

@@ -263,7 +264,7 @@ const settings = [
  "Context Enabled",
  "Hex Color Preview Enabled",
 ] as const;
-const DecoratorSettings = ({ isOpen, onClose }: RoamOverlayProps) => {
+const DecoratorSettings = ({ isOpen, onClose, extensionAPI }: RoamOverlayProps) => {


We'll need to fix the type here: RoamOverlayProps<{ extensionAPI: OnloadArgs["extensionAPI"]}>

I went a different route and removed the addCommand from toggleFeature
It made more sense to me to not "turn on and off" the feature every decoration save.
It also removes the need for type updates. And this makes more sense to me as well, as extensionAPI shouldn't be required for Decorations
3eb2707#diff-ea4e85188fe94af5ae70fed25db2d64551b1b568075d213927ed8f69a97fb202

mdroidian · 2023-03-26T03:59:27Z

Console error:

Uncaught TypeError: parseRoamMarked is not a function
    at callback (decorators.tsx:482:27)
    at callback (createHashtagObserver.js:12:13)
    at Array.forEach (<anonymous>)
    at createHTMLObserver (createHTMLObserver.js:8:27)
    at createHashtagObserver (createHashtagObserver.js:5:94)
    at toggleFeature (decorators.tsx:473:119)
    at onClick (decorators.tsx:335:7)
...

Maybe this isn't actually from the code change. Looks like something similar exists in the current version.

helpers.ts:120 Uncaught TypeError: i is not a function
    at callback (6a693c5a-69ce-4ef4-842d-c096939dbf20:2:1646319)
    at callback (6a693c5a-69ce-4ef4-842d-c096939dbf20:2:1245300)
    at Array.forEach (<anonymous>)
    at t.default (6a693c5a-69ce-4ef4-842d-c096939dbf20:2:1244712)
    at t.Z (6a693c5a-69ce-4ef4-842d-c096939dbf20:2:1245189)
    at _n (6a693c5a-69ce-4ef4-842d-c096939dbf20:2:1646081)
    at onClick (6a693c5a-69ce-4ef4-842d-c096939dbf20:2:1644114)
...

Steps to replicate:

Turn off all decorations
Disable Workbench
Reload Graph
Enable Workbench
Toggle all decorations. Save.

dvargas92495 · 2023-03-26T12:19:10Z

Yea this looks like a current bug from when the context decorator is enabled - I'd try to ignore this for now as its a separate issue

src/features/deepnav.tsx

mdroidian · 2023-03-30T00:06:33Z

for b21a052

Removed the toggle right/left sidebar, as those are built into Roam now.

Also, let me know how you feel about code formatting

I went with that because if felt more readable to me than the 200+ lines from the Prettier extension.

mdroidian · 2023-03-30T00:09:13Z

Issues found with current jumpNav (Hot Keys)

X - Expand/Collapse block tree to a certain level, specified by the following numeric key press
- number press adds number to block

And these appear to not work:

S - Add shortcut to page to left sidebar
z - Toggle Unlinked Refs
y - Toggle Queries
b - Jump to the bottom of page
- jumps to top of page
q - Toggle View type
- toggles to document, but not back to bullet

mdroidian · 2023-03-30T01:22:12Z

Well here's an unfortunate side effect of addCommand
When a command is invoke that uses the command palette sets focus of the block, then the cursor disappears.
Using a hot key, this problem does not occur.

Added timeout here: 4629860

But I feel it is bordering an unreasonable length of time.

…into command-palette-commands-389

dvargas92495 · 2023-03-30T14:40:37Z

I went with that because if felt more readable to me than the 200+ lines from the Prettier extension.

The useful part of the prettier extension is the standardization, not necessarily the decisions it comes with by default. The problem with making exceptions is that someone else working on the team will eventually edit the file, hit save, then prettier will auto format.

If you disagree with prettier defaults, the scalable solution would be to change the prettier rules, so that it gets formatted automatically. If it's unable to be encoded, then the opinion is subjective, which means it will inevitably be overwritten by somebody else's subjective experience, and now time on the PR is being spent on code formatting instead of code content.

Added timeout here

Man the problem here is I think this API from Roam is actually not very reliable - ideally we don't need a setTimeout at all. This should be fine for now

Gonna start reviewing PR now

dvargas92495

My biggest question is why did we remove all of the hotkeys to the commands?

The goal of the task was to make all of the commands have configurable hotkeys, while still retaining the default value we had so that there's no interruption in functionality

dvargas92495 · 2023-03-30T14:43:39Z

src/index.ts

    });

    return () => {
-      FEATURES.forEach(({ module }) => module.toggleFeature(false));
+      FEATURES.forEach(({ module }) => module.toggleFeature(false, extensionAPI));


One meta comment for future PRs - Big refactors like this are more digestable for reviewers as small as possible. This also helps on the engineering front as well.

I still struggle with this mightily (just look at some SamePage commits). But we should always ask ourselves: What is the smallest PR that we could submit that accomplishes our goal?

For example, PR 1 could have just been this file + 1 module. The rest of the modules will ignore the param and we would be able to iterate on convention before applying it to the rest of the modules

Ah, smaller PR's makes sense.

But at the same time, I was discovering it as I went. So I wouldn't have know that the first PR wouldn't have had to be overwritten / changed.

And when I think:

What is the smallest PR that we could submit that accomplishes our goal?

If the goal is to migrate all workbench commands to command palette, that first PR wouldn't have achieved that.

I guess I should have spent more time scoping the problem.

Making a note to clarify more on Monday 👍

dvargas92495 · 2023-03-30T14:49:46Z

src/features/alert.tsx

+      addCommand(
+        {
+          label: "Create New Alert",
+          callback: createNewAlert,
+        },
+        extensionAPI


No need to change for this PR, but a note for future reference.

In typescript, when there's more than one parameter I often prefer a single object instead of multiple params. So for example, I probably would have done:

addCommand( { label: "Create New Alert", callback: createNewAlert, extensionAPI }, )

The reason being is life becomes way easier for consumers:

Order of parameters no longer matters

We could freely make any of the parameters optional

Easier to refactor in the future: adding/removing parameters doesn't involve changing all consumers in most cases

Parameters are named, giving the consumer extra context on what the function expects

src/features/article.tsx

src/features/dailyNotesPopup.tsx

dvargas92495 · 2023-03-30T14:57:27Z

src/features/deepnav.tsx

@@ -382,6 +370,7 @@ const endNavigate = () => {
  if (document.body.classList.contains(NAVIGATE_CLASS)) {
    document.body.classList.remove(NAVIGATE_CLASS);
  }
+  window.removeEventListener("keydown", keyDownListener);


we're using isNavigating to control whether we do deepnav stuff or not, so seems like we should be able to add/remove this event listener during just the toggleFeature method

This is additionally poor on my part because the listener is added on activeDeepNav() and toggleFeature()

So are you saying it is better to have an event listener always on vs only turning it on when needed?

It's more a comment that isNavigating is already doing this work. If we want to do the smart listener approach you suggest (this is probably better, but harder to get right), then we should prob get rid of isNavigating since it will add confusion

dvargas92495 · 2023-03-30T14:58:04Z

src/features/deepnav.tsx

    endNavigate();
    document.getElementById(STYLE_ID)?.remove?.();
-    document.removeEventListener("keydown", keyDownListener, true);
+    window.removeEventListener("keydown", keyDownListener);


why did we switch from document to window?

Unintentional mistake.

dvargas92495 · 2023-03-30T15:02:03Z

src/features/workBench.ts

+
+export const removeCommand = (label: string, extensionAPI: OnloadArgs["extensionAPI"]) => {
+  extensionAPI.ui.commandPalette.removeCommand({label});
+}


With big files like this, one tactic I like to do is leave a comment like this:

This is the only substantive change to the file, the rest just add extensionAPI as an argument to addCommand. Any other differences I flag below

Sorry, I don't follow.
That would be a comment in the code or on the commit?

On the commit to make it easier for reviewers looking over big files how to read it going in

src/features/jumpNav.tsx

mdroidian · 2023-03-30T17:23:38Z

I went with that because if felt more readable to me than the 200+ lines from the Prettier extension.

The useful part of the prettier extension is the standardization, not necessarily the decisions it comes with by default. The problem with making exceptions is that someone else working on the team will eventually edit the file, hit save, then prettier will auto format.

If you disagree with prettier defaults, the scalable solution would be to change the prettier rules, so that it gets formatted automatically. If it's unable to be encoded, then the opinion is subjective, which means it will inevitably be overwritten by somebody else's subjective experience, and now time on the PR is being spent on code formatting instead of code content.

This makes sense, but if someone else were to contribute to this code, how would they be made aware of the prettier / format on save convention?

mdroidian · 2023-03-30T17:25:08Z

Speaking of which, I'm not using the "on save feature". Should I be?
I just an uneasy feeling when I go to edit a few lines by the commit shows quite a few more changes than I made.

dvargas92495 · 2023-03-30T19:56:29Z

how would they be made aware of the prettier / format on save convention?

This is somewhat implicit by seeing prettier in a repo in the industry at this point. Sortof like if you see a testing library you can assume that the project is running tests in Github in some capacity and they should all be passing before merging

Speaking of which, I'm not using the "on save feature". Should I be?

I highly recommend it, it's life changing 👍

I just an uneasy feeling when I go to edit a few lines by the commit shows quite a few more changes than I made.

That's okay bc of two reasons:

Prettier only does whitespace changes - it does not affect semantic reasoning
Because of 1, you can review PRs by checking "hide whitespace changes" to ignore those changes when reviewing

mdroidian · 2023-04-01T04:02:23Z

A few notes / questions on default hotkeys:

DNP Jump Date Forward / Back conflict with the inbuild Roam Cycle Version
alt-shift-p doesn't work for me (in Privacy Mode)

As a matter of fact, I can't set any hotkey using the UI to alt-shift-p.
I did check another program, however, and alt-shift-p worked, so it doesn't seem to be my system that is the issue.
Could you check if it works for you?

How do you want to handle the default hotkeys for Hot Keys (jumpNav.tsx)?

The docs say

All the hot keys in this module are initiated with a modifier + j + another character. So Alt+j, Ctrl+j and CMD+j are all equivalent in this context. The final character, pressed in succession after modifier + j, is what determines the action taken.

I don't believe we can set more than one hotkey as a default.

I believe the code in weekly-notes.ts actually allowed for both hotkeys because .isIOS is "true if client is iphone, ipad or ipod"

I coded it to stay in line with the docs

To jump to this week's page, hit Alt+w or Ctrl+Shift+W (Mac Only).

dvargas92495 · 2023-04-01T15:57:52Z

DNP Jump Date Forward / Back conflict with the inbuild Roam Cycle Version

How do you want to handle the default hotkeys for Hot Keys (jumpNav.tsx)?

Ok so I'm going to double back and what I said before and make an exception for this and jumpnav. Let's remove the defaults, and be sure to make announcements in #roam-js and twitter, emphasizing that the reason why we removed the defaults here are so that users could configure them in Roam's hot key panel.

alt-shift-p doesn't work for me (in Privacy Mode)

Works for me - Sometimes chrome extensions could conflict with hot keys

mdroidian · 2023-04-01T18:35:47Z

It might be prudent to not merge this until the docs have been updated.
Also, I have another idea to toss your way on Monday that relates to this.

Works for me - Sometimes chrome extensions could conflict with hot keys

🤦‍♂️ That was it!

migrate tutorial commands to command pallete

d0ccc5a

mdroidian mentioned this pull request Mar 23, 2023

migrate commands to extensionAPI RoamJS/roamjs-components#3

Merged

dvargas92495 reviewed Mar 23, 2023

View reviewed changes

src/features/tutorials.tsx Outdated Show resolved Hide resolved

mdroidian added 6 commits March 23, 2023 14:05

move commands to tutorials.tsx

ae8823a

pass extensionAPI to toggleFeature

d20b071

include removeCommand to newAddCommand return

50c04dd

migrate alert commands to command palette

8aa7028

update extensionAPI param type

ba608a4

remove newAddCommand, migrate workBench commands

3ccadf2

dvargas92495 reviewed Mar 24, 2023

View reviewed changes

src/index.ts Show resolved Hide resolved

mdroidian added 2 commits March 25, 2023 17:05

migrate decorator command to command palette

d4d78f7

minor updates

ccf5232

dvargas92495 reviewed Mar 26, 2023

View reviewed changes

mdroidian added 2 commits March 26, 2023 12:18

decouple toggleFeature and toggleDecorations

3eb2707

remove file

3d3ccae

mdroidian mentioned this pull request Mar 26, 2023

Decorated Blocks > Context Enabled Console error on first run #393

Open

migrate 3+ modules addCommands to command palette

e8fb9ec

This was referenced Mar 26, 2023

Privacy Mode: Improve UX #394

Open

Format Converter: command palette doesn't work #395

Closed

mdroidian added 4 commits March 27, 2023 11:58

migrate DNP Popup commands

31a12e3

minor changes

5dfb015

fix toggleFocus function error

654a756

migrate 3 more modules to command palette

7451e61

mdroidian commented Mar 28, 2023

View reviewed changes

src/features/deepnav.tsx Show resolved Hide resolved

mdroidian added 2 commits March 29, 2023 13:00

add getHTMLElementFromUid

f1dff63

use activeElement to handle .blur

ed1661a

migrate jumpNav (Hot Keys) to commandPalette

b21a052

mdroidian mentioned this pull request Mar 30, 2023

Hot Keys: Issues found #396

Open

mdroidian changed the title ~~migrate commands to command palette~~ migrate commands to command palette (Closes #389) Mar 30, 2023

mdroidian added 2 commits March 29, 2023 18:11

add setTimeout to cursor placements + formatting

f9bfbe5

update timeout to 300

4629860

mdroidian added 4 commits March 29, 2023 18:30

remove Deep Nav from dropdown

830cdd8

0.0.1

0a42854

type update

dadf190

Merge branch 'main' of https://github.com/mdroidian/roamjs-workbench …

239152e

…into command-palette-commands-389

mdroidian marked this pull request as ready for review March 30, 2023 02:49

dvargas92495 reviewed Mar 30, 2023

View reviewed changes

DRY refactor commands

dac7911

mdroidian added 2 commits March 31, 2023 13:56

move event listener

630cc94

add previous default hotkeys

c1d368c

Remove default for DNP Jump Date Forward/Backward

73cfcfe

dvargas92495 approved these changes Apr 3, 2023

View reviewed changes

dvargas92495 merged commit dec57eb into RoamJS:main Apr 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate commands to command palette (Closes #389) #391

migrate commands to command palette (Closes #389) #391

mdroidian commented Mar 23, 2023 •

edited

Loading

mdroidian commented Mar 23, 2023

dvargas92495 left a comment

mdroidian commented Mar 26, 2023

dvargas92495 Mar 26, 2023

mdroidian Mar 26, 2023 •

edited

Loading

mdroidian commented Mar 26, 2023

dvargas92495 commented Mar 26, 2023

mdroidian commented Mar 30, 2023

mdroidian commented Mar 30, 2023 •

edited

Loading

mdroidian commented Mar 30, 2023

dvargas92495 commented Mar 30, 2023

dvargas92495 left a comment

dvargas92495 Mar 30, 2023

mdroidian Mar 30, 2023

dvargas92495 Mar 30, 2023

dvargas92495 Mar 30, 2023

dvargas92495 Mar 30, 2023

mdroidian Mar 30, 2023

dvargas92495 Mar 30, 2023

dvargas92495 Mar 30, 2023

mdroidian Mar 30, 2023

dvargas92495 Mar 30, 2023

mdroidian Mar 30, 2023

dvargas92495 Mar 30, 2023

mdroidian commented Mar 30, 2023

mdroidian commented Mar 30, 2023

dvargas92495 commented Mar 30, 2023

mdroidian commented Apr 1, 2023 •

edited

Loading

dvargas92495 commented Apr 1, 2023

mdroidian commented Apr 1, 2023

migrate commands to command palette (Closes #389) #391

migrate commands to command palette (Closes #389) #391

Conversation

mdroidian commented Mar 23, 2023 • edited Loading

mdroidian commented Mar 23, 2023

dvargas92495 left a comment

Choose a reason for hiding this comment

mdroidian commented Mar 26, 2023

Choose a reason for hiding this comment

mdroidian Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

mdroidian commented Mar 26, 2023

dvargas92495 commented Mar 26, 2023

mdroidian commented Mar 30, 2023

mdroidian commented Mar 30, 2023 • edited Loading

mdroidian commented Mar 30, 2023

dvargas92495 commented Mar 30, 2023

dvargas92495 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

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

mdroidian commented Mar 30, 2023

mdroidian commented Mar 30, 2023

dvargas92495 commented Mar 30, 2023

mdroidian commented Apr 1, 2023 • edited Loading

dvargas92495 commented Apr 1, 2023

mdroidian commented Apr 1, 2023

mdroidian commented Mar 23, 2023 •

edited

Loading

mdroidian Mar 26, 2023 •

edited

Loading

mdroidian commented Mar 30, 2023 •

edited

Loading

mdroidian commented Apr 1, 2023 •

edited

Loading