ENG-737 Use Node Color to style Node Tags #414
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sid597
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughReplaces per-tag membership Set with a tag-to-style map built during refresh, normalizing colors. The node tag popup observer now checks the map and, when present, renders the popup and applies inline styles (color, padding, borderRadius) to the tag span. Adds formatHexColor import. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Init as initializeObserversAndListeners
participant Refresh as Refresh Logic
participant DOM as NodeTagPopupButtonObserver
participant UI as renderNodeTagPopupButton
rect rgb(240,248,255)
note over Init: Initialization
App->>Init: Initialize observers/listeners
end
rect rgb(235,250,240)
note over Refresh: Build tag-to-style map
Init->>Refresh: Refresh discourse data
Refresh->>Refresh: Iterate discourseNodes
Refresh->>Refresh: If node.tag and canvas color → formatHexColor
Refresh->>Refresh: discourseTagToStyle[tag] = { color }
end
rect rgb(250,245,235)
note over DOM: Observe tag spans and clicks
DOM->>DOM: On tag element detected
DOM->>Refresh: Lookup discourseTagToStyle[tag]
alt Style exists
DOM->>UI: renderNodeTagPopupButton(tag, ...)
DOM->>DOM: Apply inline styles (color, padding, borderRadius)
else No style
DOM->>DOM: Skip popup/styling
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (2)
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
47-47: ConfirmformatHexColorguarantees a valid return (and non-throw).If it can throw on malformed input, guard calls (see next comment’s diff) so a bad color on one node doesn’t break styling for all. Also verify this import path won’t create a circular dep at runtime.
128-134: Ensure color actually applies to the clickable link and add visible affordance.Roam often colors the inner
<a>; setting only the wrapper<span>may be overridden. Also, padding/borderRadius aren’t visible without a border/background.- const style = discourseTagToStyle[tag.toLowerCase()]; - if (style) { - renderNodeTagPopupButton(s, discourseNodes, onloadArgs.extensionAPI); - s.style.color = style.color; - s.style.padding = "2px 4px"; - s.style.borderRadius = "4px"; - } + const style = discourseTagToStyle.get + ? discourseTagToStyle.get(tag.toLowerCase()) + : (discourseTagToStyle as any)[tag.toLowerCase()]; + if (style) { + renderNodeTagPopupButton(s, discourseNodes, onloadArgs.extensionAPI); + // ensure container and link both adopt the color + s.style.color = style.color; + const link = s.querySelector("a.rm-page-ref") as HTMLAnchorElement | null; + if (link) link.style.color = style.color; + // make the tag visually distinct + s.style.padding = "2px 4px"; + s.style.borderRadius = "4px"; + s.style.border = "1px solid currentColor"; + }Optional follow-up: to reflect color changes without navigation, after each
refreshDiscourseNodeCache()call, re-style existing tags by iteratingdocument.querySelectorAll('span.rm-page-ref--tag[data-tag]')and applying the same block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/roam/src/utils/initializeObserversAndListeners.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
sid597
changed the title
| discourseNodes = getDiscourseNodes(); | ||
| discourseTagSet = new Set( | ||
| discourseNodes.flatMap((n) => (n.tag ? [n.tag.toLowerCase()] : [])), | ||
| discourseTagToStyle = discourseNodes.reduce( |
There was a problem hiding this comment.
We should reuse the existing refreshConfigTree. No need to re-invent the wheel 😀
| const style = discourseTagToStyle[tag.toLowerCase()]; | ||
| if (style) { | ||
| renderNodeTagPopupButton(s, discourseNodes, onloadArgs.extensionAPI); | ||
| s.style.color = style.color; |
There was a problem hiding this comment.
Could we put this color/style directly on the Blueprint button? That way it would all be in one place. Also, we could use tailwind/classes as much as possible so users could adjust the styles themselves.
There was a problem hiding this comment.
sorry which button? We have to add the color the node tag in the block right?
There was a problem hiding this comment.
Ah right, my bad. I was thinking of the popover.
sid597
changed the base branch from
eng-798-node-tags-breaks-color-highlighter
to
graphite-base/414
|
Why are we not using the "button-like" CSS styling? I think it's much more appealing and makes clear you should hover mouse over: |
Summary by CodeRabbit