[ENG-259] Create Observer and UI pattern #138
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughA new feature has been introduced that allows users to trigger and interact with a searchable popover menu for inserting discourse nodes into a Roam Research block by typing "@". This involves the creation of a new React component for the menu, the addition of a keyboard event listener to detect the trigger, and updates to the initialization and teardown of event listeners. The menu supports keyboard and mouse interactions, updates block content upon selection, and tracks analytics events. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Textarea
participant discourseNodeSearchTriggerListener
participant NodeSearchMenu
participant ExtensionAPI
User->>Textarea: Types "@"
Textarea->>discourseNodeSearchTriggerListener: keydown event
discourseNodeSearchTriggerListener->>NodeSearchMenu: renderDiscourseNodeSearchMenu()
User->>NodeSearchMenu: Types search term / navigates / selects item
NodeSearchMenu->>ExtensionAPI: Update block content with selected node
NodeSearchMenu->>NodeSearchMenu: Track selection event
NodeSearchMenu-->>User: Menu closes, block updated
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
170-174: Globalkeydownlistener should be scopedAttaching to
documentcaptures every keystroke while the menu is open.
Instead, attach the listener to the menu container or usePopover’sonKeyDownprop to keep the handler local and avoid unintended side-effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx(1 hunks)apps/roam/src/index.ts(2 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
renderDiscourseNodeSearchMenu(234-252)
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Generally good stuff!
A couple use cases we should handle before merging
- insert node and keep typing (don't create a new block on enter)
- dealing with text after
@ - remove node menu when
@is gone
|
How goes the progress on this? |
|
I'm still working on this. there's some cursor position bug I still can't figure out yet. |
trangdoan982
force-pushed
the
eng-259-observer
branch
from
284f844 to
b733791
Compare
|
@mdroidian https://www.loom.com/share/1582b3a10eb44abdbd955c16536caa98 new checkbox for filtering node types during search and a few test cases |
|
Good stuff. But what happens to the UI when a graph has 9 or 10 nodes? Could you test this on |
|
@mdroidian i set the popover menu to fixed height will scrollable y so it will look the same and users just need to scroll down to see the results: scroll.search.menu.mov |
|
Having to scroll each time is probably something we want to avoid. Likely we'll want to widen the entire popover and have the check boxes limited to one or two rows, then scroll horizontal. Or remove the grouping and hide the filters until the user wants them. We can consider this out of scope for the current PR and ask Johnny his opinion. |
|
i did start with responsive sizing for the menu, but it somehow created a bug that mess with the popover's position. So i'd go with overflow-x-auto for the filters. |
|
this is fantastic! I see the utility of having filters given a user might want to only see a particular set of nodes and exclude others from the search. My intuition though is that the filtering action will a less frequent operation and could be relegated to the dg settings menu so that users have the option to customize node search to their needs, but the menu itself will not be cluttered with the filter options. I'd be content with that solution and then if users request to have filter settings more easily accessible we can explore designs such as these below: For now I'm content with having filter options in the settings menu and not in the node search menu. |
|
@jsmorabito my instinct is that user might want to change up the filter type when they query quite often. I'm imagining a flow where users would switch the type up as they type down their paper outline. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
🔥 A few minor changes requested.
Also, I don't believe we should be merging this to main as is. If we want an anchor I'll create a eng-235-create-dg-summoning-menu to merge to.
| } | ||
| }); | ||
|
|
||
| // Analytics |
trangdoan982
changed the base branch from
main
to
eng-235-create-dg-summoning-menu
would this be something similar to a search with advanced options like we see in obsidians search widget? I can definitely see a use for having some advanced search that let's you have a more specific query which can persist in the sidebar while your outline is in the main window |
|
@jsmorabito i was referring to this: if there's an UI pattern that can be clean and allow people to chose only 1 node type (i imagine it useful if there are many node types). But that's more a nice-to-have. I don't think we need to overkill this with an advanced filter option for now. |
|
Gotcha! I made a project in Linear where we can continue to expand on this idea: https://linear.app/discourse-graphs/project/advanced-search-4b947899da16/overview Feel free to add more notes there. Also added a ticket for mockup up this functionality: https://linear.app/discourse-graphs/issue/DES-49/mock-up-advanced-search |
|
But yea, for now this node search menu is good without the filters (in the menu itself at least) |
|
@mdroidian addressed your PR comments. I also implemented the filter toggle that Johnny mocked. I made sure the focus remains even when you interact with the filter menu. test.filter.mov |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Good stuff!
One thing we should add in the future is capturing clicks and preventing the default so they don't remove focus. (use case: user wants to click the filter button or a checkbox but misses)
https://www.loom.com/share/6a852a8007464f709700f41571c0646c
None of this is blocking.
trangdoan982
merged commit 962f5ae
into
eng-235-create-dg-summoning-menu
* [ENG-259] Create Observer and UI pattern (#138) * create event listener for @ key * fix ui * address PR comments * cur progress * current progress * tested yayyy * add the checkbox UI * fix focus issue * rm logs * address PR comments * address PR comments * [ENG-261] Query function for DG summoning menu (#150) * cur progress * current progress * rm logs * address PR comments * address PR comments * query works * address PR comments * address PR comments * sm change * [ENG-299] Trigger setting for DG summoning menu (#162) * setting menu done * trigger works correctly * clean up * address PR comment * optimize getFocusedBlock() * address nit comments * fix bug (#166) * address PR comments
https://www.loom.com/share/16c24a6cc7ad4ca6aef6624ffc097bae
Summary by CodeRabbit