-
Notifications
You must be signed in to change notification settings - Fork 3
[ENG-1080 & ENG-1067] Change key figure behaviors and styles #574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
also addresses ENG-1067 |
📝 WalkthroughWalkthroughThe changes introduce image source (imageSrc) handling to the Discourse node creation and rendering pipeline. Dynamic node sizing based on image presence is now integrated via calcDiscourseNodeSize, replacing fixed defaults. DiscourseNodeShape rendering reorders title/type to appear after the image with added padding. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx(2 hunks)apps/obsidian/src/components/canvas/utils/convertToDiscourseNode.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/obsidian/src/components/canvas/utils/convertToDiscourseNode.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
apps/obsidian/**
📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)
apps/obsidian/**: Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Follow the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for UI and code styling
Use Lucide and custom Obsidian icons alongside detailed elements to provide visual representation of features in platform-native UI
Files:
apps/obsidian/src/components/canvas/utils/convertToDiscourseNode.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
🧠 Learnings (2)
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/obsidian/src/components/canvas/utils/convertToDiscourseNode.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
📚 Learning: 2025-11-25T00:52:27.779Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-11-25T00:52:27.779Z
Learning: Applies to **/*.{ts,tsx} : When refactoring inline styles, use tailwind classes
Applied to files:
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
🧬 Code graph analysis (1)
apps/obsidian/src/components/canvas/utils/convertToDiscourseNode.ts (2)
apps/obsidian/src/components/canvas/stores/assetStore.ts (1)
addWikilinkBlockrefForFile(30-51)apps/obsidian/src/utils/calcDiscourseNodeSize.ts (1)
calcDiscourseNodeSize(22-67)
🔇 Additional comments (6)
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (1)
311-314: LGTM: Comment clarifies Tailwind class dependencies.The reformatted comment clearly documents the relationship between these Tailwind classes and constants in
nodeConstants.ts, which is helpful for maintainability.apps/obsidian/src/components/canvas/utils/convertToDiscourseNode.ts (5)
21-21: LGTM: Import added for dynamic node sizing.The import of
calcDiscourseNodeSizeenables dynamic sizing based on content, which is a good improvement over fixed dimensions.
144-148: LGTM: Image source handling is clean and properly scoped.The image source is correctly obtained using
getResourcePathand is only computed when an image file exists. The logic properly handles the optional nature of the image.
185-193: LGTM: Function signature properly updated for image support.The
imageSrcparameter is correctly typed as optional (imageSrc?: string) and properly integrated into the function signature using named parameters.
201-210: Verify thatcalcDiscourseNodeSizehandles undefinedimageSrcgracefully.The dynamic size calculation replaces fixed defaults, which is a good improvement. However, ensure that
calcDiscourseNodeSizeproperly handles the case whenimageSrcisundefined.Looking at the provided code snippet from
apps/obsidian/src/utils/calcDiscourseNodeSize.ts:21-66, the function checksif (!imageSrc || !nodeType?.keyImage)and returns text-only dimensions, so undefined values are handled correctly. No action needed.
219-224: LGTM: Shape properties correctly updated with dynamic values.The calculated dimensions (
w,h) andimageSrcare properly assigned to the shape properties, ensuring the node is created with the correct size and image reference.
https://www.loom.com/share/f2553f62439b4c5eba7037ddec0c1541
Summary by CodeRabbit
New Features
UI/Style Improvements
✏️ Tip: You can customize this high-level summary in your review settings.