ENG-2037 Add tabbed node card context menu to Roam tldraw - #1272
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3475f5dcb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f453d08 to
cc8000e
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
A few issues:
- Why are the headers on the left this time? Let's center them as per the design.
https://www.loom.com/share/75d4ddc684594ac7898ee2cc31a62e38
- A couple issues noticed here when creating relations
@sid597 would you mind recording a manual video for this, please? Let's try to think of different surfaces to manually stress test this and walk through theme one by one.
| if (!created) return; | ||
| const util = editor.getShapeUtil(created); | ||
| if (util instanceof DiscourseNodeUtil) { | ||
| await util.createExistingRelations({ shape: created }); |
There was a problem hiding this comment.
There's a flag for this (autoCanvasRelations). Let's make sure we respect it.
| className="min-w-0 flex-1 truncate text-sm" | ||
| title={text} | ||
| > | ||
| {text} |
There was a problem hiding this comment.
Let's use roam's react.BlockString to give a consistent feel to referenced nodes.
|
@sid597 There's also an issue with creating new relations. They are not showing up in the new discourse context. See if you can come up with a solution for that or if it is deeper rooted in an existing caching mechanism. |
526d512 to
3fb1906
Compare
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
2 participants
eng-2037.mp4
Overrides tldraw's StylePanel for Roam: selecting a single node card shows a Context tab (default) listing all discourse relations grouped by relation type, with +/− buttons that add or remove the related node on the canvas, and a Styling tab that reuses the stock styling controls. Regular tldraw shapes and multi-selection keep the default style panel. Adding a node also draws its existing relation arrows via createExistingRelations; removing deletes the node shape and its arrows — relation data in Roam is never modified.