search

RoamJS / workbench

https://roamjs.com/extensions/workbench
The Unlicense
286 stars 35 forks source link

Use a git submodule for roam-navigator #310

Closed mgsloan closed 3 years ago

mgsloan commented 3 years ago

As suggested in #87, this will make it more straightforward to keep track of what roam42 has changed, and easier to update after I make changes to roam-navigator

You can use submodules in glitch via the console. See e.g: https://blog.mozvr.com/lessons-from-hacking-glitch/#integrating-react

mgsloan commented 3 years ago

I am also curious about the .includes change for ipad - https://github.com/roamhacker/roam42/commit/9ea683b6046c926a793be2e445edad0cf4457929#diff-7918699bc7ee248c64d6e6c4b4742ffd7e61d7a2504e81ac089260d3bd82ac01R498 - what is the text like on ipad?

I also omitted the disabling of the getStack function in https://github.com/roamhacker/roam42/commit/8df7ee7e06d443e17d2085998c451e939041b1af#diff-7918699bc7ee248c64d6e6c4b4742ffd7e61d7a2504e81ac089260d3bd82ac01

mgsloan commented 3 years ago

Also, can fork roam-navigator on github to your account, and update the submodule url.

One thing to note is that when rebasing the roam42 commit(s) atop a newer version of roam-navigator, it should be associated with a new branch pushed to github. Otherwise, old commits will get GCed by github and break the SHA references. Git submodules are a little gnarly. It's too bad that github doesn't treat them as GC roots.

TfTHacker commented 3 years ago

I am also curious about the .includes change for ipad - 9ea683b#diff-7918699bc7ee248c64d6e6c4b4742ffd7e61d7a2504e81ac089260d3bd82ac01R498 - what is the text like on ipad?

I also omitted the disabling of the getStack function in 8df7ee7#diff-7918699bc7ee248c64d6e6c4b4742ffd7e61d7a2504e81ac089260d3bd82ac01

Mike, on my iPad it seemed ALL PAGES was returning extra characters. So doing just a comparison of === "ALL PAGES" was not working. by using includes, it just checks if ALL PAGES is in the string. The desktop was not doing this. Could be a difference in how Safari on iOS reads the DOM element.

TfTHacker commented 3 years ago

Mike,

Here is what I am worried about. Roam navigator is a chrome extension. I am using the roam navigator code from roam/js along with other roam42 features that have keyboard shortcuts. So a lot of the code I add doesn't have any value to your chrome extension, and if I leave the code as is from your rep, it won't work with Roam42. So I am not sure how to resolve this.

I could do it as a fork of course. If you consider moving to roam/js with navigator, I would pull it from 42 and just send people to your extension. Have you give any thought to that?

mgsloan commented 3 years ago

Thanks for the info! Yeah, could be browser differences in how text of a node is computed.

I haven't thought of turning it into a roam/js thing. I haven't really looked into how that works. My rough understanding is that it's per-graph, I think I like the convenience of a chrome extension.

An alternative approach is that I will just manage a roam42 branch in my repo, that has a commit representing the roam42 changes. I'll then rebase it, and push the updates to you like in https://github.com/roamhacker/roam42/pull/316