Skip to content

feat: add CodeEditor component#780

Merged
shariquerik merged 8 commits into
frappe:mainfrom
shariquerik:code-editor
Jun 14, 2026
Merged

feat: add CodeEditor component#780
shariquerik merged 8 commits into
frappe:mainfrom
shariquerik:code-editor

Conversation

@shariquerik

@shariquerik shariquerik commented Jun 13, 2026

Copy link
Copy Markdown
Member
image

Coverage: 56.68% (+0.15% vs main)

@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Concerns — solid component; two things to look at before merge.

The editor itself is well done — reuses InputLabelingProps/InputSize/variant, lazy-loads CodeMirror behind frappe-ui/code-editor like frappe-ui/editor, careful a11y (Esc to escape the Tab trap, ARIA on contentDOM), and change (blur) vs update:modelValue (live) are genuinely distinct. Good.

  • Editor is untested. tests/CodeEditor.test.ts only covers CodePreview sanitization — it even concedes jsdom can't mount the editor. All the real logic (blur-commit change, variant/size/readonly swaps, prefix/suffix diff-sync, language race, overflow emit) ships with no test. That's what a Cypress component test is for, and it's the convention here — 38 components have a colocated <Name>.cy.ts; none use a tests/ subfolder. Add a CodeEditor.cy.ts.
  • src/components/CodeEditor/types.tsCodeEditorVariant redeclares InputVariant verbatim ('subtle' | 'outline' | 'ghost'). Import the shared union instead (Textarea/types.ts:1,9 does) so the code field can't drift from the inputs it sits next to. API smell Figma UI Kit #3.

Nits:

  • types.tslanguage?: string is plain string even though CodeLanguage is defined and exported. CodeLanguage | (string & {}) keeps autocomplete while still allowing fall-through.
  • CodeEditor.vue is 693 lines; the theme + highlightStyle literals (~200 lines inside onMounted) would read better as a sibling theme.ts.
  • overflow emit + maxHeight are new public surface. Fine if it's truly for the field wrapper's expand/collapse, but worth confirming a consumer couldn't derive it from the scroll box itself before adding an event name outside the canonical set.

Replace the tests/ subfolder with a colocated CodeEditor.cy.ts covering the
editor's real behaviour (blur-commit change vs live update, Esc tab-trap exit,
prop diff-sync, readonly/variant/size swaps, labeling ARIA, overflow emit) and
move the CodePreview sanitization cases to a sibling CodePreview.test.ts.
- Reuse the shared InputVariant union (via Exclude<…, 'ghost'>) instead of
  redeclaring it, so the code field can't drift from the inputs beside it.
- Drop the ghost variant: a borderless code surface reads as plain text and
  loses the affordance that it's an editable field. Removed from the type, the
  theme fragment, the variants story, the playground knob, and the docs.
- Type language as CodeLanguage | (string & {}) to keep autocomplete while
  still allowing arbitrary keys.
- Extract the ~200 lines of theme + highlightStyle literals into a sibling
  theme.ts (taking the lazy-imported CodeMirror modules as params so the
  static graph stays editor-free).
Drop the maxHeight style-injection prop (no precedent in the library and at
odds with P10's styling-via-CSS rule). The cap is now set by the consumer
through the --cm-max-height custom property on the root, which cascades into
CodeMirror and the base theme already reads. Add the data-slot="control"
styling hook the component was missing.

The overflow emit stays: the cap crossing needs the ResizeObserver to measure,
so a consumer can't derive it from CSS.
@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shariquerik — checked whether the prior concerns were addressed.

Looks good — all three points from the last pass are resolved.

  • Editor is now tested: CodeEditor.cy.ts covers the real logic — blur-commit change vs live update:modelValue, Esc-to-commit, prop-sync no-echo, runtime readonly swap, data-* hooks, aria wiring, and the overflow crossing. This was the main blocker.
  • types.ts:36variant now derives from the shared union (Exclude<InputVariant, 'ghost'>) instead of redeclaring it. Drift closed.
  • language is now CodeLanguage | (string & {}), and the ~200 lines of theme/highlight literals moved out to theme.ts.

Nice direction: the old maxHeight prop became a --cm-max-height CSS hook (types.ts:60) — styling via CSS per P10 rather than a style prop. That's the way to keep the surface from growing. The overflow emit stays, and the justification holds: a consumer genuinely can't measure the cap crossing from CSS, so reporting it via an event is fair.

One thing worth a glance, not blocking: readonly is fed into useInputLabeling as disabled, so a read-only editor renders data-disabled='true'/data-state. Intentional styling reuse, but if a consumer ever styles disabled and read-only differently they'll collide — fine to leave as-is for now.

Use the canonical `disabled` state prop (P4) to match TextInput/Textarea
instead of the non-standard `readonly`. Disabled now also greys the
surface (flat gray box, no hover/focus chrome, dimmed content) so the
field reads as inert like the other inputs, not just blocks editing.
@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shariquerik — third pass, no new concerns.

Looks good — stable since the last pass; everything still holds and the surface got tighter.

  • API stays tight: variant: Exclude<InputVariant, 'ghost'>, size: InputSize, language: CodeLanguage | (string & {}), all from the shared unions (types.ts). No style props — the height cap is the --cm-max-height CSS hook (P10).
  • readonly is gone entirely now; only disabled remains. That closes the disabled/readonly data-* collision flagged last time — cleaner than keeping both.
  • CodeEditor.cy.ts (156 lines) covers the real logic: live update:modelValue vs blur-commit change, Esc-to-commit, prop-sync no-echo, runtime disabled swap, data-*/aria wiring, and both overflow crossings.
  • Component logic reads carefully: langSeq drops stale async language builds, the prefix/suffix diff keeps the caret put on prop-sync, and the destroyed flag guards the async onMounted against unmount-mid-load.

CI all green (Cypress 1-3, build, coverage, test). Nothing blocking.

@shariquerik shariquerik merged commit 3dfbd4d into frappe:main Jun 14, 2026
6 checks passed
@shariquerik shariquerik deleted the code-editor branch June 14, 2026 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant