[ENG-872] Add ability to change font style and sizing for dn #551
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis pull request updates the Tldraw canvas UI to replace the StylePanel with SharePanel for discourse-specific tools, and adds dynamic font styling support throughout the discourse node system by introducing configurable size and fontFamily properties that propagate through shape rendering, sizing calculations, and text measurement utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Shape as DiscourseNodeShape
participant Calc as calcDiscourseNodeSize
participant Measure as measureNodeText
participant Render as Text Rendering
Shape->>Calc: pass size, fontFamily
Calc->>Measure: propagate size, fontFamily
Measure->>Measure: compute fontSize from FONT_SIZES<br/>compute fontFamily from FONT_FAMILIES
Measure->>Render: apply computed fontSize & fontFamily<br/>title: fontSize | subtitle: fontSize × 0.75
Render->>Render: measure & layout text with styles
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/obsidian/src/utils/measureNodeText.ts (1)
49-49: Extract magic number 0.75 to a named constant.The subtitle font size multiplier
0.75appears in multiple places (lines 49, 89, 298) as a magic number. This makes the relationship between title and subtitle sizing less explicit.Extract to a named constant for clarity and maintainability:
+const SUBTITLE_FONT_SIZE_RATIO = 0.75; + export const measureNodeText = ({ title, subtitle, size = "s", fontFamily = "draw", }: { title: string; subtitle: string; size?: TLDefaultSizeStyle; fontFamily?: TLDefaultFontStyle; }): { w: number; h: number } => { const fontSize = FONT_SIZES[size]; const fontFamilyValue = FONT_FAMILIES[fontFamily]; - const subtitleFontSize = fontSize * 0.75; + const subtitleFontSize = fontSize * SUBTITLE_FONT_SIZE_RATIO;Apply the same constant in DiscourseNodeShape.tsx at line 298.
Also applies to: 89-89, 298-298
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx(2 hunks)apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx(7 hunks)apps/obsidian/src/constants.ts(2 hunks)apps/obsidian/src/utils/calcDiscourseNodeSize.ts(2 hunks)apps/obsidian/src/utils/measureNodeText.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/obsidian/src/constants.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/obsidian/src/constants.ts
🔇 Additional comments (4)
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (2)
39-41: Good: Font styling props properly integrated.The addition of
sizeandfontFamilyprops to theDiscourseNodeShapeis well-structured with appropriate types, prop definitions, and sensible defaults. The props flow correctly through the shape lifecycle.Also applies to: 62-63, 74-75
273-274: Verify tldraw exports for FONT_SIZES and FONT_FAMILIES.Same import verification issue as in measureNodeText.ts. This file imports
FONT_SIZESandFONT_FAMILIESfrom tldraw.See verification request in measureNodeText.ts review. Ensure consistent import source across all files.
apps/obsidian/src/utils/calcDiscourseNodeSize.ts (1)
10-10: No issues found — "draw" is a valid TLDefaultFontStyle value.The valid TLDefaultFontStyle values in tldraw 3.14.2 are: 'draw', 'mono', 'sans', 'serif'. The code correctly uses "draw" as the default font family, so no changes are required.
apps/obsidian/src/utils/measureNodeText.ts (1)
13-18: The review comment is based on incorrect reasoning and should be disregarded.The web search confirmed that tldraw 3.14.2 does export
FONT_SIZESandFONT_FAMILIES. The codebase investigation reveals that whileconstants.tsdoes define a localFONT_SIZESat line 95, it is not exported and is not used anywhere—making it unused dead code. The files that useFONT_SIZES(measureNodeText.tsandDiscourseNodeShape.tsx) correctly import it from tldraw with no ambiguity. The import statement inmeasureNodeText.tsis valid and correct.Likely an incorrect or invalid review comment.
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/obsidian/src/constants.ts (1)
85-105: Remove unused localFONT_SIZESand stale TODO / SVG font comments.This block introduces:
- A local
FONT_SIZES: Record<TLDefaultSizeStyle, number>- A TODO pointing to an old tldraw PR
- Commented SVG_FONT_FAMILY notes
Given that:
FONT_SIZEShere is not exported or used in this file- Other code (e.g.
measureNodeText.ts) already importsFONT_SIZESandFONT_FAMILIESdirectly fromtldrawthis local definition and related comments are dead/duplicated state and a likely source of confusion about the single source of truth for font sizes.
Recommend deleting the local
FONT_SIZES, the TODO, and the commented SVG_FONT_FAMILY block, and then removing the now‑unusedTLDefaultSizeStyleimport.Example diff:
-import { TLDefaultSizeStyle } from "tldraw"; +// import { TLDefaultSizeStyle } from "tldraw"; // (no longer needed if local FONT_SIZES is removed) @@ -// TODO REPLACE WITH TLDRAW DEFAULTS -// https://github.com/tldraw/tldraw/pull/1580/files - -const TEXT_PROPS = { +const TEXT_PROPS = { lineHeight: 1.35, fontWeight: "normal", fontVariant: "normal", fontStyle: "normal", padding: "0px", maxWidth: "auto", }; -const FONT_SIZES: Record<TLDefaultSizeStyle, number> = { - m: 25, - l: 38, - xl: 48, - s: 16, -}; -// // FONT_FAMILIES.sans or tldraw_sans not working in toSvg() -// // maybe check getSvg() -// // in node_modules\@tldraw\tldraw\node_modules\@tldraw\editor\dist\cjs\lib\app\App.js -// const SVG_FONT_FAMILY = `"Inter", "sans-serif"`;Then remove the
TLDefaultSizeStyleimport entirely once the localFONT_SIZESis gone.apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (1)
15-15: SharePanel override cleanly routes toDiscourseToolPanel; verify it now exposes font controls.The new
SharePaneloverride:
- Correctly uses
useTools/useIsToolSelectedinside the component, matching tldraw’s hooks patterns.- Shows
DiscourseToolPanelwhen either discourse tool is active, and otherwise defers to<DefaultSharePanel />, so default share UI isn’t lost.This looks structurally sound. The remaining question is UX/functionality: given the PR goal (change font size and style for discourse nodes), please confirm that
DiscourseToolPanelnow exposes controls wired toDiscourseNodeShape’ssizeandfontFamilyprops (and triggers any necessary node‑size recalculation) so users actually gain font editing capabilities via this panel.Also applies to: 380-397
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (1)
156-171: Ensure font changes also trigger node size recalculation (not just image changes).In
loadNodeData, the call tocalcDiscourseNodeSizeis guarded bydidImageChange, so the size recalculation currently only runs when the key image changes:if (didImageChange) { const { w, h } = await calcDiscourseNodeSize({ ... , size: shape.props.size, fontFamily: shape.props.fontFamily }); // ... }If
sizeorfontFamilyare changed via the UI without any image change, this effect alone won’t recalculatew/h, which could leave text clipped or cramped unless some other code path (e.g. inDiscourseToolPanel) explicitly callscalcDiscourseNodeSizeand updates the shape dimensions.Please confirm that:
- The code that updates
shape.props.size/shape.props.fontFamilyalso recomputesw/h(e.g. by callingcalcDiscourseNodeSize), or- You extend this effect (or add another) so that font‑only changes result in a size recalculation, including for nodes without images.
If no such path exists yet, adding a dedicated recalculation triggered by font props would be important for UX.
Also applies to: 222-246, 260-271
🧹 Nitpick comments (1)
apps/obsidian/src/utils/measureNodeText.ts (1)
13-18: Dynamic font size/family measurement is aligned with the rendered node; consider a small defensive fallback.
- Importing
FONT_SIZES/FONT_FAMILIESand threadingsize/fontFamilythroughmeasureNodeTextgives you consistent measurement withDiscourseNodeShape’s runtime styling.- Using
subtitleFontSize = fontSize * 0.75and matching theopacity-80behavior for the subtitle keeps the visual + measured layout in sync.Given the TS types,
sizeandfontFamilyshould always be valid keys. If you want extra robustness against corrupted data, you could add a small fallback:- const fontSize = FONT_SIZES[size]; - const fontFamilyValue = FONT_FAMILIES[fontFamily]; + const fontSize = FONT_SIZES[size] ?? FONT_SIZES.s; + const fontFamilyValue = FONT_FAMILIES[fontFamily] ?? FONT_FAMILIES.draw;Not required, but it would harden the function against unexpected values.
Also applies to: 33-35, 39-46, 47-50, 77-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx(2 hunks)apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx(7 hunks)apps/obsidian/src/constants.ts(2 hunks)apps/obsidian/src/utils/calcDiscourseNodeSize.ts(2 hunks)apps/obsidian/src/utils/measureNodeText.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/obsidian/src/constants.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/obsidian/src/constants.ts
🔇 Additional comments (4)
apps/obsidian/src/constants.ts (2)
83-84: Auto‑save delay remains consistent with existing behavior.Keeping
DEFAULT_SAVE_DELAYat 500ms aligns with the debounce logic inTldrawViewComponentand looks fine.
106-112: DEFAULT_STYLE_PROPS is reasonable as a central typography default.The structure (spreading
TEXT_PROPSthen overridingfontSize,fontFamily,width, andpadding) is clear and gives you a single default source for text styling. Looks good, assuming downstream components (e.g.DiscourseNodeShape) either use these values or are deliberately diverging.apps/obsidian/src/utils/calcDiscourseNodeSize.ts (1)
10-19: PlumbingsizeandfontFamilythrough node size calculation looks correct.
- Extending
CalcNodeSizeParamswithsize?: TLDefaultSizeStyleandfontFamily?: TLDefaultFontStyle, and defaulting them to"s"/"draw"in the function signature, matches the newDiscourseNodeShapedefaults.- Forwarding these into
measureNodeTextis exactly what’s needed so typography choices influence measured width/height.No issues spotted here; the function remains backward‑compatible while enabling dynamic typography.
Also applies to: 30-41
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (1)
9-15: Wiringsize/fontFamilythrough shape props and rendering looks correct.
- Extending
DiscourseNodeShapewithsize: TLDefaultSizeStyleandfontFamily: TLDefaultFontStyle, backed byDefaultSizeStyle/DefaultFontStyleinstatic props, matches tldraw’s style patterns.- Defaults of
"s"and"draw"line up with the defaults used incalcDiscourseNodeSizeandmeasureNodeText.- Using
FONT_SIZES/FONT_FAMILIESat render time and applyingfontSize/fontFamilyvia inline styles to the title and the 0.75x subtitle keeps the visual output consistent with measurement.This part of the integration looks solid.
Also applies to: 39-42, 62-64, 74-76, 273-275, 286-303
trangdoan982
force-pushed
the
eng-872-add-ability-to-change-font-style-and-sizing-for-dn
branch
from
673f4fe to
7192937
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Missed this comment in my review, so re-reviewed.
Original review video:
#551 (review)
| title: file.basename, | ||
| nodeTypeId: nodeTypeId, | ||
| size: "s", | ||
| fontFamily: "draw", |
There was a problem hiding this comment.
Default needs to be Sans throughout. This was requested when we did this in Roam. Double check that when a new node is added to the canvas or created for the firs time that is defaults to sans. This was a issue in Roam.
It is also default m in Roam, but we can leave that until we consolidate further.
There was a problem hiding this comment.
My bad, default should be s for nodes. Relations might be m
#432
|
next action: address PR review |
https://www.loom.com/share/18e6cc45d59640479091d9722ca846de?focus_title=1&muted=1&from_recorder=1
Summary by CodeRabbit
New Features
UI Changes
Defaults