ENG-2183 Build the new setting-item row component - #1421
Conversation
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:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
trangdoan982
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
882dd98 to
d280e8c
Compare
Add one shared settings row — scope indicator, label, always-visible
description, control — and route every panel-based row through it.
Descriptions move out of the hover popover that dismissed before its doc links
could be clicked (ENG-2080). The fix is structural: the description renders as a
sibling of the label, never a descendant, so a link inside it can no longer
toggle the row's control.
The rewire happens at SettingTitle, the one helper all five Base*Panels funnelled
label and description through, so ~70 call sites are unchanged. Flag rows swap
Checkbox for Switch to match the design.
Scope is derived from the wrapper rather than passed per call site, because the
wrapper already binds the setter that decides who a value is written for. Rows
that cannot derive it pass `scope` explicitly.
Layout is Tailwind, which resolves because Roam itself bundles Tailwind. Anything
depending on Tailwind's preflight is hand-rolled instead: `border-b` and `border`
set only a width, and `* { border-style: solid }` is absent, so those borders
never paint. This repo's own `neutral-*` theme colours are likewise absent from
Roam's build and are mirrored as CSS variables.
Supersedes ENG-2186's SettingsDrillDownRow, whose two callers now compose the
shared row with a trailing drill-down summary.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A node type's settings are written to that node type's own page block props, so the whole graph sees them — the agreed badge model has two values, `just you` and `everyone`, and a per-node scope was never one of them. Drops the invented `nodeType` scope and its icon; the nine affected rows now badge as `global`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…verflow Review feedback on the row component: - Separators ran between sibling rows only, so they stopped at every group boundary. Each row now owns its bottom edge. - BaseNumberPanel put `w-24` on the NumericInput, but Blueprint applies that class to the outer ControlGroup while the input keeps its intrinsic width; the box shrank and the input spilled past it (Advanced > Queries, and the same control in Export options). The width now sits on a wrapper and the input fills it. - The Export dialog is too narrow for the trailing layout, so rows take a `compact` presentation there: no scope badge, control below the label, tighter vertical spacing — the shape the options had before this component. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The group heading's 2px rule already frames the group; a 1px hairline after its last row put two rules at one boundary. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Hairlines under rows were inconsistent — present on flat rows, dropped after a group's last row, absent on rows not yet on SettingItemRow — so they told the eye nothing reliable. A line now appears in exactly one place, under a section heading. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tity group Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
044c940 to
e723797
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e7237978cf
ℹ️ 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".
| --dg-neutral-dark: #1f1f1f; | ||
| --dg-neutral-light: #f1f1f1; | ||
| --dg-border: rgb(229, 231, 235); |
There was a problem hiding this comment.
When Roam uses a dark or custom theme, these root variables still resolve to the fixed light-theme palette, and .dg-setting-row__label explicitly applies #1f1f1f; consequently every migrated setting label bypasses the host foreground color and can become unreadable. Inherit Roam's colors or use established Blueprint/Roam theme variables instead of defining a new neutral palette.
AGENTS.md reference: apps/roam/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| const isAssociated = typeof control === "function"; | ||
| // Description is a sibling of the label: nested, its doc links would toggle the control (ENG-2080). | ||
| const LabelTag = isAssociated ? "label" : "div"; |
There was a problem hiding this comment.
When control is passed as a React element, this converts the visible label to a plain div. The migrated Color, Node search menu trigger, and Personal override inputs are all supplied this way and expose no explicit accessible label, so clicking their label no longer focuses the input and assistive technology encounters unnamed fields. Pass these simple controls as render callbacks and forward controlId, as KeyboardShortcutInput already does, or provide an equivalent explicit association.
Useful? React with 👍 / 👎.
2 participants
Stacked on #1374 (ENG-2185).
Reviewer brief
SettingItemRowrenders every setting the same way: scope badge, label, always-visible description, control. Doc links in descriptions are clickable because the description no longer sits in a hover popover (ENG-2080).compactmode hides the badge and stacks the control below the label for narrow hosts like the Export dialog.SettingItemRow.tsx. The label is a<label>only when the control accepts the generated id. The description is a sibling of the label, not a child. Both keep a click on the text from toggling the wrong control.settingKeysstamps thedata-setting-idanchor that search (ENG-2184) scrolls to.SettingKeycapInputis a narrow, centred, monospace input for shortcut keys.DiscourseNodeMenuandDiscourseNodeSearchMenuuse it.SettingDrillDownSummaryshows the current value with a chevron and pushes a route segment. It replacesSettingsDrillDownRowfrom ENG-2188.BlockPropSettingPanels.tsxrender through the row. That movesNodeConfig,PreferencesGeneral,QuerySettings,PageGroupPanel, andExportOptionsonto it. Per-node settings are badgedglobalbecause they live on the node type's page and apply to the whole graph.SettingsDescriptionis now an inline div instead of the roamjs hover popover. Rows not yet onSettingItemRowchange appearance too.Verification
tsc --noEmitand eslint pass forapps/roamwith no warnings on added lines. Production build completes with 0 errors.Loom video
Not recorded yet.
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: the ticket leaves call-site migration to ENG-2187. This PR migrates the generic panels and about a dozen rows anyway, because the component has no consumer to test against otherwise. The remaining rows stay with ENG-2187.Local delegated full review
🤖 Generated with Claude Code