feat(TextEditor): prose-v3 typography and code block improvements#620
feat(TextEditor): prose-v3 typography and code block improvements#620netchampfaris merged 7 commits intomainfrom
Conversation
…uage label
- Add prose-v3 typography modifier (zero paragraph margins, 8px grid spacing)
- Default editorClass changed to prose-v3
- Remove prose-v2 CSS (replaced by plugin-based approach)
- Code block: GitHub light/dark themes based on data-theme
- Code block: 12px font, 1.7 line-height, border, border-radius
- Code block: language selector always visible in edit mode
- Code block: static language label in readonly mode (reactive via editor.on('update'))
- Blockquote: 2px border-inline-start via style.css override
- Empty paragraph height: 20px in prose-v3
- Add PROSE_V3.md documenting design decisions and migration strategy
… branch - Switch all editor instances from prose-sm to prose-v3 - Point frappe-ui to frappe/frappe-ui#better-prose-styles (frappe-ui PR: frappe/frappe-ui#620)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces a new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/TextEditor/components/CodeBlockComponent.vue`:
- Around line 42-46: The mounted() hook attaches an anonymous listener via
this.editor.on('update', ...) but never removes it, causing a memory leak;
refactor to store the callback in a stable reference (e.g.
this.handleEditorUpdate) and use that same function to unsubscribe in
beforeUnmount() by calling this.editor.off('update', this.handleEditorUpdate),
ensuring you update this.isEditable inside the handler (keep the reference name
consistent with the component methods/fields such as mounted, beforeUnmount, and
isEditable).
In `@src/components/TextEditor/PROSE_V3.md`:
- Around line 24-36: Update the PROSE_V3.md docs to match the implemented
values: change the documented line-height from 1.6 to 1.7 to match plugin.js,
update "Empty paragraph" from ≈24px to 20px to match the height set in
style.css, and change the documented li spacing from 2px to 4px to match the li
margin set in plugin.js; reference the specific keys "line-height" in plugin.js,
the empty paragraph "height" in style.css, and the "li" margin in plugin.js when
making these edits.
- Around line 22-25: Update the fenced code block in PROSE_V3.md that contains
"Base unit: 8px / Empty paragraph: 14px × 1.6..." to include a language
identifier (e.g., use ```text or ```plaintext) on the opening fence so the
linter stops flagging it; only modify the opening fence to ```text (or
```plaintext) and leave the block contents unchanged.
In `@tailwind/plugin.js`:
- Around line 393-401: The comment for the v3 prose settings is inconsistent
with the implementation: PROSE_V3.md and the inline comment state a 1.6
line-height (14px × 1.6 ≈ 22.4px) but the actual config sets lineHeight: '1.7'
under the v3 css block (fontSize: '14px', fontWeight: 420, lineHeight: '1.7').
Fix by either updating the comment/PROSE_V3.md to reflect 1.7 (and the new
calculated px) or change the lineHeight value to '1.6' so the code matches the
comment and documentation; locate the v3 css object (v3 -> css -> { fontSize,
fontWeight, lineHeight }) to make the edit.
- Around line 444-533: The file defines blockquote twice so the second
declaration overrides the first and drops border/color settings; merge the two
by removing the first blockquote object and copying its borderInlineStartWidth,
borderInlineStartStyle, borderInlineStartColor, color and any fontStyle into the
later blockquote block that contains margin/padding/quotes, then move the
adjacent selectors 'blockquote p:first-of-type::before' and 'blockquote
p:last-of-type::after' next to the merged blockquote to keep the no-quote rules
together; ensure the resulting single blockquote contains all properties
(borderInlineStartWidth, borderInlineStartColor, borderInlineStartStyle,
fontStyle, color, marginTop, marginBottom, paddingInlineStart, quotes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 371869f5-0ecd-47d1-877b-65e18b366438
📒 Files selected for processing (5)
src/components/TextEditor/PROSE_V3.mdsrc/components/TextEditor/TextEditor.vuesrc/components/TextEditor/components/CodeBlockComponent.vuesrc/components/TextEditor/style.csstailwind/plugin.js
| mounted() { | ||
| this.editor.on('update', () => { | ||
| this.isEditable = this.editor.isEditable | ||
| }) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does TipTap editor.on() return an unbind function?
💡 Result:
No, TipTap's editor.on does not return an unbind function. It follows a standard EventEmitter pattern where editor.on(event, callback) adds the listener but returns the editor instance (this) for chaining. To unbind, use editor.off(event, callback) with the exact same callback reference. This is shown in the official documentation with examples like: const onUpdate = => { ... }; editor.on('update', onUpdate); editor.off('update', onUpdate);
Citations:
- 1: https://tiptap.dev/docs/editor/api/events
- 2: https://github.com/ueberdosis/tiptap/blob/main/packages/core/src/Editor.ts
- 3: Add better types for event emitter ueberdosis/tiptap#1898
- 4: ueberdosis/tiptap@54e85fd
Missing event listener cleanup — potential memory leak.
The editor.on('update', ...) listener is registered in mounted() but never removed. This will leak memory if the component is mounted/unmounted multiple times. TipTap uses a standard EventEmitter pattern: to unbind, call editor.off(event, callback) with the same callback reference in beforeUnmount().
🐛 Proposed fix: store callback and call editor.off() on unmount
data() {
return {
isEditable: this.editor.isEditable,
+ onUpdate: null,
}
},
mounted() {
- this.editor.on('update', () => {
+ this.onUpdate = () => {
this.isEditable = this.editor.isEditable
- })
+ }
+ this.editor.on('update', this.onUpdate)
},
+ beforeUnmount() {
+ if (this.onUpdate) {
+ this.editor.off('update', this.onUpdate)
+ }
+ },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/TextEditor/components/CodeBlockComponent.vue` around lines 42
- 46, The mounted() hook attaches an anonymous listener via
this.editor.on('update', ...) but never removes it, causing a memory leak;
refactor to store the callback in a stable reference (e.g.
this.handleEditorUpdate) and use that same function to unsubscribe in
beforeUnmount() by calling this.editor.off('update', this.handleEditorUpdate),
ensuring you update this.isEditable inside the handler (keep the reference name
consistent with the component methods/fields such as mounted, beforeUnmount, and
isEditable).
| // blockquote: left border, receded color, no italic, no quote marks | ||
| blockquote: { | ||
| 'border-inline-start-width': '2px', | ||
| borderInlineStartColor: 'var(--ink-gray-3)', | ||
| borderInlineStartStyle: 'solid', | ||
| fontStyle: 'normal', | ||
| color: 'var(--ink-gray-6)', | ||
| }, | ||
| 'blockquote p:first-of-type::before': { content: 'none' }, | ||
| 'blockquote p:last-of-type::after': { content: 'none' }, | ||
|
|
||
| // paragraphs: zero margin — user controls spacing with empty paragraphs | ||
| p: { | ||
| marginTop: '0', | ||
| marginBottom: '0', | ||
| }, | ||
|
|
||
| // headings: marginTop creates section break (32/24px), | ||
| // marginBottom keeps heading close to its content (8px, proximity) | ||
| // h1/h2: full weight + darkest; h3-h5: softer weight + stepped-back color | ||
| h1: { | ||
| fontSize: em(20, 14), | ||
| marginTop: '32px', | ||
| marginBottom: '8px', | ||
| lineHeight: '1.3', | ||
| }, | ||
| h2: { | ||
| fontSize: em(18, 14), | ||
| marginTop: '32px', | ||
| marginBottom: '8px', | ||
| lineHeight: '1.35', | ||
| }, | ||
| h3: { | ||
| fontSize: em(16, 14), | ||
| marginTop: '24px', | ||
| marginBottom: '8px', | ||
| lineHeight: '1.4', | ||
| }, | ||
| h4: { | ||
| fontSize: em(14, 14), | ||
| marginTop: '24px', | ||
| marginBottom: '8px', | ||
| lineHeight: '1.45', | ||
| }, | ||
| h5: { | ||
| fontSize: em(13, 14), | ||
| marginTop: '24px', | ||
| marginBottom: '8px', | ||
| lineHeight: '1.45', | ||
| }, | ||
|
|
||
| // element after heading gets no extra top margin | ||
| 'h1 + *': { marginTop: '0' }, | ||
| 'h2 + *': { marginTop: '0' }, | ||
| 'h3 + *': { marginTop: '0' }, | ||
| 'h4 + *': { marginTop: '0' }, | ||
| 'h5 + *': { marginTop: '0' }, | ||
|
|
||
| // lists: small outer margin (4px), tight internal spacing | ||
| ul: { | ||
| marginTop: '4px', | ||
| marginBottom: '4px', | ||
| paddingInlineStart: '1.5em', | ||
| }, | ||
| ol: { | ||
| marginTop: '4px', | ||
| marginBottom: '4px', | ||
| paddingInlineStart: '1.5em', | ||
| }, | ||
| li: { | ||
| marginTop: '4px', | ||
| marginBottom: '4px', | ||
| }, | ||
| 'li p': { | ||
| marginTop: '4px', | ||
| marginBottom: '4px', | ||
| }, | ||
| 'ul ul, ul ol, ol ul, ol ol': { | ||
| marginTop: '4px', | ||
| marginBottom: '4px', | ||
| }, | ||
|
|
||
| // blockquote: breathing room (16px), no italic, no quotes | ||
| blockquote: { | ||
| marginTop: '16px', | ||
| marginBottom: '16px', | ||
| paddingInlineStart: '1em', | ||
| fontStyle: 'normal', | ||
| quotes: 'none', | ||
| }, |
There was a problem hiding this comment.
Duplicate blockquote property — second declaration overrides the first.
The static analyzer correctly identified that blockquote is defined twice (lines 445-451 and 527-533). The second declaration will completely override the first, losing the borderInlineStartColor and color properties.
🐛 Proposed fix: merge blockquote declarations
Remove the first blockquote block (lines 445-451) and merge its properties into the second one:
- // blockquote: left border, receded color, no italic, no quote marks
- blockquote: {
- 'border-inline-start-width': '2px',
- borderInlineStartColor: 'var(--ink-gray-3)',
- borderInlineStartStyle: 'solid',
- fontStyle: 'normal',
- color: 'var(--ink-gray-6)',
- },
- 'blockquote p:first-of-type::before': { content: 'none' },
- 'blockquote p:last-of-type::after': { content: 'none' },
-
// paragraphs: zero margin — user controls spacing with empty paragraphsThen update the second blockquote block to include all properties:
// blockquote: breathing room (16px), no italic, no quotes
blockquote: {
marginTop: '16px',
marginBottom: '16px',
paddingInlineStart: '1em',
fontStyle: 'normal',
quotes: 'none',
+ 'border-inline-start-width': '2px',
+ borderInlineStartColor: 'var(--ink-gray-3)',
+ borderInlineStartStyle: 'solid',
+ color: 'var(--ink-gray-6)',
},Also move the ::before/::after selectors to be adjacent to the merged blockquote block.
🧰 Tools
🪛 Biome (2.4.10)
[error] 445-451: This property is later overwritten by an object member with the same name.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tailwind/plugin.js` around lines 444 - 533, The file defines blockquote twice
so the second declaration overrides the first and drops border/color settings;
merge the two by removing the first blockquote object and copying its
borderInlineStartWidth, borderInlineStartStyle, borderInlineStartColor, color
and any fontStyle into the later blockquote block that contains
margin/padding/quotes, then move the adjacent selectors 'blockquote
p:first-of-type::before' and 'blockquote p:last-of-type::after' next to the
merged blockquote to keep the no-quote rules together; ensure the resulting
single blockquote contains all properties (borderInlineStartWidth,
borderInlineStartColor, borderInlineStartStyle, fontStyle, color, marginTop,
marginBottom, paddingInlineStart, quotes).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/TextEditor/PROSE_V3.md (1)
155-159:⚠️ Potential issue | 🟡 MinorAdd language identifier to code block.
The linter flags this code block as missing a language identifier. Since it describes a deployment sequence in plain text, consider using
textorplaintext.✏️ Suggested fix
-``` +```text 1. Deploy code (prose-v3 default) ← can still revert code-only, zero data risk 2. Run forward migration ← insert marked empty paragraphs 3. Monitor ← if issues, run rollback + revert code ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TextEditor/PROSE_V3.md` around lines 155 - 159, Update the Markdown code block that lists the deployment steps so it includes a language identifier (use "text" or "plaintext"); locate the fenced code block starting with "1. Deploy code (prose-v3 default)" in PROSE_V3.md and change the opening fence from ``` to ```text so the linter recognizes it as plain text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/TextEditor/PROSE_V3.md`:
- Around line 155-159: Update the Markdown code block that lists the deployment
steps so it includes a language identifier (use "text" or "plaintext"); locate
the fenced code block starting with "1. Deploy code (prose-v3 default)" in
PROSE_V3.md and change the opening fence from ``` to ```text so the linter
recognizes it as plain text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a0cd704-650c-41a5-b2fb-caf7d4713104
📒 Files selected for processing (1)
src/components/TextEditor/PROSE_V3.md
to fix paragraph overlap bug
|
Some observations:
|
… non-breaking - Revert default editorClass back to prose-sm (non-breaking for existing users) - Restore prose-v2 CSS that was removed (restores backward compatibility) - prose-v3 remains as an opt-in modifier, not enforced by default
2 participants
Summary
This PR adds a new
prose-v3typography modifier for TextEditor content and improves code block presentation in light and dark themes.It does not make
prose-v3the default yet.TextEditorstill defaults toprose-sm.What changed
typography.v3intailwind/plugin.jsprose-p-spacingas a compatibility modifier for legacy paragraph spacingCodeBlockComponent.vuestyling and language UIstyle.cssoverrides for readonly empty paragraphs and blockquote border specificityDesign decisions
prose-v3is explicit: paragraphs have no default margins.prose-v3styling is opt-in and scoped; existing typography defaults stay unchanged.prose-p-spacingprovides a transition path for older content.Notes
PROSE_V3.mdnote and moved the rationale here.prose-v3by default is a follow-up decision.