shakedown round 2: docs + chainable Draggable.onMouseDown#82
Merged
Conversation
…ling Closes shakedown C11 (#64). Stop excluding `onMouseDown` from `DraggableProps`. The component now calls the consumer's onMouseDown FIRST (synchronously, before its internal handleDragPress) so app-level concerns like "raise window on press" / "focus this widget on click" / press-time analytics can observe every press. Consumer can call `event.captureGesture(...)` themselves to preempt the drag — captureGesture is first-call-wins (PR #81 / A21), so a consumer that captures here owns the subsequent motion + release and Draggable's internal capture becomes a no-op. Side-effect contract: the press-time z-bump (raise on press) still fires regardless of whether the consumer preempts. "Press is press" — the z-bump is a UI signal independent of "is this a drag." To suppress all press behavior including the z-bump, set `disabled` instead. Implementation matches the existing onDragStart/onDrag/onDragEnd ref pattern: stash the consumer handler in a ref, read fresh closure on each press, keep the useCallback identity stable across prop changes. Type contract: `DraggableProps` now omits only `position | top | left` (was previously also omitting `onMouseDown`). The `onMouseDown` prop's JSDoc spells out the chain order + the captureGesture preemption escape hatch + the z-bump exception. Tests: extended `DraggableProps` describe with a satisfies-check that the type accepts a consumer onMouseDown. The chain ordering itself is thin (one ref read + one call) — exercised by the existing `pnpm demo:drag` / `pnpm demo:scratchpad` smoke tests; the underlying captureGesture first-call-wins semantic that makes preemption work is already covered by mouse-event.test.ts (PR #81). Docs: `docs/components/draggable.md` props table gets the new row + the "owned internally" sentence is updated to reflect the new forward-chain behavior. Co-Authored-By: Mark <psyhik17@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
Closes shakedown B6 (#59). Promote the existing one-line hint into a full Layout pitfalls section: cause (Box default flexShrink: 1, CSS-flex spec compliant), symptom (titlebars / footers / status rows silently disappear when a sibling has large natural size), fix (flexShrink={0} on chrome that should always render), heuristic ("if collapsing the element would make the UI broken or unrecognizable, it should be flexShrink={0}"). Also notes the same pitfall affects wrapping <Text> in fixed-height containers (links to A1 territory until that's properly fixed). Cross-link from docs/components/box.md's flexShrink prop row to the new section so consumers reading the props table get to the pitfall without first having to know the concepts page exists. Co-Authored-By: Mark <psyhik17@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
Closes shakedown D14 (#67). When a consumer writes <Text> nested inside <Text>, the reconciler rewrites the inner one to ink-virtual-text (reconciler.ts:289-310). Virtual-text has no Yoga node — the whole nested-Text tree becomes a single flex leaf, sized by squashTextNodesToSegments which flattens descendants into one measurement string. This is invisible to consumers today: they reach for nested Text expecting Box-like flex semantics (multiple flex children), get single-leaf semantics (one flex child), and get bitten when a sibling pushes width pressure that collapses or wraps the whole nested tree. New "Nested vs sibling Text" section in docs/concepts/text.md explains: - the reconciler rewrite (with file:line ref) - what virtual-text means for layout (one Yoga leaf, not many) - when nested makes sense (style spans inside flowing text) - when sibling Texts in a Box make sense (independent layout, gap, individual flexShrink, hit-testing) - the known-issue forward-link to #51 (silent truncation under certain flex configurations) until that's properly fixed Cross-link from docs/components/text.md's "Nested styles" example so the props-table reader sees the gotcha without first knowing the concepts page exists. Co-Authored-By: Mark <psyhik17@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
Closes shakedown D15 (#68). The scratchpad demo's `<Draggable width=W height=H borderStyle bg flexDirection="column">` shape with content directly inside, plus the `onMouseDown` + `stopImmediatePropagation` + `captureGesture({onUp})` pattern for in-window buttons, IS the canonical "build a window- shaped element on yokai" recipe today. It was a demo, not labeled as the pattern; consumers reverse-engineered it (termos did this). New docs/patterns/window.md mirrors the structure of existing pattern docs (modal.md, confirmation-dialog.md): full code, "How it works" walkthrough, "Variations" (no close button, multiple windows, constrained drag, modal-window, resizable corners gap), "Known caveats" (interactive-children pattern, fixed dimensions required, implicit titlebar drag), "Related" (links to Draggable, Modal, scratchpad source, A4 issue #56, A22 issue #72). Marked as INTERIM — explicit forward-link to the proper <Window> primitive (issue #56) so consumers know there's a long-term answer coming. Cross-link from docs/components/draggable.md Related section so draggable readers see the window recipe. Co-Authored-By: Mark <psyhik17@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
Closes shakedown E16 (#69). When a consumer uses link: to depend on a local yokai checkout (not a monorepo workspace, not an npm install), their react and yokai's react resolve to DIFFERENT module instances. React's per-module dispatcher singleton check fails, useContext returns null for every yokai-provided context (FocusContext, TerminalSizeContext, etc.), and core hooks silently no-op: - useFocus returns no-op ref + isFocused: false - useTerminalViewport returns no dimensions - Cursor declarations don't fire - FocusGroup doesn't cycle on Tab The fix is to force the consumer's react and react-reconciler to link from yokai's node_modules, so both sides share one module instance. New "Local-checkout development (link:)" section in docs/getting-started/install.md covers: - the package.json shape (4 link: entries) - the why (peer-dep dedup model + React's per-module singleton) - the symptoms (so consumers recognize the bug from observed behavior) - a verification step (compare symlink targets via ls) - reference to termos's package.json as the working example Section placed AFTER "Monorepo workspace" so the canonical-install flows (npm tarball, GitHub tag, monorepo workspace) appear in discoverability order; link: development is the niche case at the bottom. Co-Authored-By: Mark <psyhik17@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
re-marked
added
the
source:shakedown
![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
yokai/docs/getting-started/install.md
Lines 7 to 10 in 17eaa97
Include the reconciler peer in install steps
The renderer package declares react-reconciler as a peer dependency and imports it at runtime (packages/renderer/src/reconciler.ts), but this peer table and the canonical install command only tell consumers to install react. In pnpm/npm configurations that do not auto-install peers, following these docs leaves @yokai-tui/renderer unable to resolve react-reconciler; the peer table and install command should include the reconciler peer as well.
ℹ️ 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".
| - **Different titlebar content**: replace the `<Text dim>` with breadcrumbs, tab bar, status indicators, etc. As long as one element captures the close-or-equivalent action via `onMouseDown` + `e.captureGesture`, the rest can stay clickable. | ||
| - **Multiple windows**: render N `<Draggable>` siblings. Each owns its own z-index (Draggable bumps `persistedZ` on press for raise-on-press). The most-recently-pressed window stays in front of the others. | ||
| - **Constrained drag**: pass `bounds={{ width, height }}` so the window can't be dragged past viewport edges. Useful when the WM should keep windows reachable. | ||
| - **Modal mode**: combine with the [Modal](./modal.md) pattern's backdrop — render a full-viewport `<Box position="absolute" backgroundColor="black" zIndex={100}>` behind the window's `<Draggable zIndex={101}>`. |
There was a problem hiding this comment.
This modal recipe relies on <Draggable zIndex={101}>, but Draggable always passes its own zIndex to the underlying Box after spreading user props, so the consumer-supplied value is ignored. With the suggested backdrop at zIndex={100}, a fresh Draggable starts around z=10 and will render behind the backdrop, making the modal window unusable; the docs need a pattern that actually raises the window above the backdrop or the component needs to expose a base z-index.
Useful? React with 👍 / 👎.
Two P2s flagged by codex:
1. **Install instructions missed react-reconciler peer.** The renderer
declares `react-reconciler` as a peer dep and imports it at
runtime (reconciler.ts), but the peer table and canonical install
command only listed react. pnpm doesn't auto-install peers; npm
only does on clean installs. Following the docs would leave
consumers with an unresolved react-reconciler at runtime.
Add react-reconciler to the peer-deps table, the npm install
command, and the GitHub-tag jsonc example. Add a note explaining
why both peers must be passed explicitly.
2. **Window pattern recommended `<Draggable zIndex={101}>` which is
ignored.** Draggable always passes its own internal zIndex
(persistedZ, starting around 10) to the underlying Box AFTER
spreading user props, so consumer-supplied zIndex is silently
dropped. With the previously-suggested backdrop at zIndex=100,
a fresh Draggable at z=10 would render BEHIND the backdrop —
modal window invisible.
Fix the pattern: give the BACKDROP a low absolute zIndex (e.g. 1)
and let Draggable's natural press-bumped z float above it. Pressing
the window once bumps it well above any low backdrop value. Note
that an explicit `baseZIndex` prop on Draggable would be a cleaner
API (forward-link to a future enhancement).
Co-Authored-By: Mark <psyhik17@gmail.com>
Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced May 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundled follow-up to the 6 codex shakedown PRs (#76-#81). 5 commits, mostly docs, one small code change.
Highlights
Test plan
Notes