Add Personal Node Menu Trigger, style Node Menu #40
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThe pull request introduces a new feature allowing users to set personalized keyboard shortcuts for triggering the Discourse Node Menu. This enhancement spans multiple files, including Changes
Sequence DiagramsequenceDiagram
participant User
participant Settings
participant NodeMenu
participant Listener
User->>Settings: Set personal trigger
Settings-->>Listener: Update trigger configuration
User->>Listener: Press keyboard shortcut
Listener->>NodeMenu: Check trigger validity
alt Personal Trigger Set
Listener->>NodeMenu: Render with personal trigger
else Personal Trigger Not Set
Listener->>NodeMenu: Render with global trigger
end
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/roam/src/settings/settingsPanel.ts (1)
19-29: Clarify refresh requirement in the UI.The description states a refresh is required after editing, which might be overlooked by users. Consider adding an inline note or tooltip near the setting to emphasize this requirement and improve user experience.
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
134-134: Good fallback for the global trigger.
globalTriggerdefaults to"\\", ensuring the node menu is still accessible even when no personal trigger has been set.Consider documenting why
"\\"is chosen as the default to enhance readability for future maintainers.
146-154: Encapsulate UI logic in a separate utility function.
handleNodeMenuRendereffectively separates rendering logic, improving readability. It might be beneficial to handle edge cases (e.g., if the selection is read-only or some states in the editor).apps/roam/src/components/DiscourseNodeMenu.tsx (2)
132-133: Improve fallback color.When
item?.canvasSettings?.coloris absent, consider using a documented default or theme-based color instead of#000.
223-277: Functionality anchored in user experience.
- The
NodeMenuTriggerComponenteffectively handles user input for personalized shortcuts, updating local settings in real time.- The blueprint
InputGroupusage for capturing key-down events is a good approach for setting custom triggers without interfering with global key events.Consider adding some validation for potential conflicts (e.g., choosing “Enter,” “Tab,” or other reserved keys), or at least surfacing a warning to the user.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/DiscourseNodeMenu.tsx(4 hunks)apps/roam/src/settings/settingsPanel.ts(2 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(2 hunks)
🔇 Additional comments (10)
apps/roam/src/settings/settingsPanel.ts (1)
2-2: Good import usage.
The NodeMenuTriggerComponent import is well-structured and follows the file's modular approach.
apps/roam/src/utils/initializeObserversAndListeners.ts (4)
32-35: Import structure is clear and consistent.
Imports for getModifiersFromCombo and renderDiscourseNodeMenu are grouped logically, which aligns well with this file's focus on node menu triggers.
36-36: Well-chosen single import.
The IKeyCombo import from @blueprintjs/core is consistent with the logic introduced below for handling personal triggers.
139-145: Avoid potential mismatch with outdated settings.
When retrieving "personal-node-menu-trigger", consider verifying its structure before casting to IKeyCombo. This helps avoid runtime errors if a malformed or legacy value is saved in the user’s local settings.
155-176: Keyboard event priority is well-handled.
- Personal trigger supersedes the global trigger, which aligns with user expectations.
- Correct usage of early returns to exit when modifiers or triggers do not match.
However, ensure that the code gracefully handles collisions between personal and global triggers if users set the same key for both.
apps/roam/src/components/DiscourseNodeMenu.tsx (5)
1-10: Effective usage of BlueprintJS components.
The newly added InputGroup, getKeyCombo, and IKeyCombo imports neatly support the personalized node menu trigger functionality.
26-27: Imports from roamjs-components.
OnloadArgs and formatHexColor imports match the usage in this file (especially for color formatting). This is a clear, modular approach.
138-153: Consistent UI design.
- Displaying item text, color icon, and the shortcut label within
MenuItemimproves user clarity. - The class names (
mr-2 h-4 w-4 select-none rounded-full) provide suitable styling, but ensure tailwind or relevant classes are consistently used across the codebase.
182-211: Concise key normalization logic.
The normalizeKeyCombo function is well-structured and uses common aliases.
213-221: Useful utility for extracting modifiers.
getModifiersFromCombo is a neat helper to parse modifiers which helps unify logic throughout the code.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation