Posting-3: Tweet-splitter#138
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a new CKEditor plugin "tweet_splitter" and accompanying stylesheet that inserts tweet-separators, computes Twitter-style per-tweet lengths (URLs/emoji rules), displays counters/numbering/arrows, customizes copy behavior, and validates tweets against a 280-character limit. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant E as CKEditor
participant P as tweet_splitter Plugin
participant D as Editor DOM
participant F as Form
U->>E: Click "Разбить на твиты"
E->>P: exec insertTweetSplitter
P->>D: Insert separator block (label + remove)
P->>P: updateTweetCounters()
P->>D: Render counters/numbering/arrows
rect rgba(230,240,255,0.2)
U-->>E: Type / paste / commands
E->>P: key / afterCommandExec / contentDom events
P->>P: scheduleTweetCountersUpdate()
P->>D: Recompute lengths & update UI
end
U->>F: Submit form
F->>P: validateTweets(editor)
alt Any tweet > 280 chars
P-->>F: Validation fails
F-->>U: Block submit / show message
else All tweets within limit
F-->>U: Submit proceeds
end
opt Copy selection
U->>E: Copy
E->>P: Clipboard hook
P->>P: Strip separators/counters, return plain text
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
static/ckeditor/ckeditor/plugins/tweet_splitter/styles.css (3)
1-4: Scope CSS: avoid overriding the editor’s global typography.Remove the global body rule to prevent unexpected font/line-height changes to all editor content. Scope styles to plugin UI only.
-body { - font-family: 'Helvetica', sans-serif; - line-height: 1.6; -}
61-73: Add visible focus styles for keyboard accessibility.Make the remove control clearly focusable, not just hoverable.
.tweet-separator-remove:hover { background-color: #ddd; } + +.tweet-separator-remove:focus-visible { + outline: 2px solid #666; + outline-offset: 2px; + border-radius: 4px; +}
61-69: Increase hit area for the remove control.Small targets hurt usability. Add minimal padding.
.tweet-separator-remove { cursor: pointer; display: inline-flex; align-items: center; line-height: 1; user-select: none; vertical-align: middle; margin-left: 5px; + padding: 2px; + border-radius: 4px; }static/ckeditor/ckeditor/plugins/tweet_splitter/plugin.js (7)
61-61: Remove stray double semicolon.- let html = editor.getSelectedHtml(true);; + let html = editor.getSelectedHtml(true);
57-73: Harden copy handler for broader browser support.Guard clipboardData and provide a plain-text fallback to avoid exceptions in constrained environments.
- editor.document.on('copy', function(evt) { + editor.document.on('copy', function(evt) { const e = evt.data.$; // нативный ClipboardEvent - e.preventDefault(); // отменяем стандартное копирование + e.preventDefault(); // отменяем стандартное копирование let html = editor.getSelectedHtml(true); // Удаляем div tweet-separator и tweet-char-counter целиком html = html.replace(/<div class="tweet-(separator|char-counter)[^>]*>[\s\S]*?<\/div>/g, ''); // Убираем все теги, оставляем только текст let text = html.replace(/<[^>]+>/g, ''); // Кладём своё - e.clipboardData.setData('text/plain', text); - e.clipboardData.setData('text/html', text); + if (e.clipboardData && e.clipboardData.setData) { + e.clipboardData.setData('text/plain', text); + e.clipboardData.setData('text/html', text); + } else if (window.clipboardData && window.clipboardData.setData) { + // IE fallback + window.clipboardData.setData('Text', text); + } });
12-17: Make the remove control accessible.Add button semantics and keyboard operability hints.
- <span class="tweet-separator-remove" title="Объединить твиты"> + <span class="tweet-separator-remove" title="Объединить твиты" role="button" tabindex="0" aria-label="Объединить твиты">
24-35: Use event delegation for remove to cover future separators and avoid re-binding.One listener on the editor document is cheaper and more reliable than per-node onclick.
- const editorDom = editor.document.$; - editorDom.querySelectorAll('.tweet-separator-remove').forEach(button => { - button.onclick = function() { - const separator = button.closest('.tweet-separator'); - if (separator) { - separator.remove(); - setTimeout(() => updateTweetCounters(editor), 100); - editor.focus(); - } - }; - }); + const nativeDoc = editor.document.$; + if (!nativeDoc._tweetSplitterRemoveBound) { + nativeDoc._tweetSplitterRemoveBound = true; + nativeDoc.addEventListener('click', (ev) => { + const btn = ev.target.closest && ev.target.closest('.tweet-separator-remove'); + if (!btn) return; + const sep = btn.closest('.tweet-separator'); + if (sep) { + sep.remove(); + scheduleTweetCountersUpdate(editor); + editor.focus(); + } + }); + nativeDoc.addEventListener('keydown', (ev) => { + if ((ev.key === 'Enter' || ev.key === ' ') && ev.target.classList && ev.target.classList.contains('tweet-separator-remove')) { + ev.preventDefault(); + ev.target.click(); + } + }); + }
84-93: Emoji/URL counting: verify browser targets or use twitter-text for spec-accurate length.Unicode property escapes may not be supported in legacy browsers and will throw at parse-time. Consider a guarded fallback or adopting twitter-text’s parsing for correctness.
Guarded fallback:
- // Эмодзи считаем за 2 символа - const emojiRegex = /[\p{Emoji_Presentation}\p{Emoji}\u200d]/gu; - text = text.replace(emojiRegex, 'xx'); + // Эмодзи считаем за 2 символа (guard for browsers without Unicode property escapes) + try { + const emojiRegex = /[\p{Emoji_Presentation}\p{Emoji}\u200d]/gu; + text = text.replace(emojiRegex, 'xx'); + } catch (_) { + // Fallback: approximate by replacing surrogate pairs and ZWJ + text = text + .replace(/[\uD800-\uDBFF][\uDC00-\uDFFF]/g, 'xx') + .replace(/\u200d/g, 'x'); // keep ZWJ light to avoid double counting + }Option: integrate twitter-text and delegate length calculations for exact parity with Twitter.
2-2: Icon config mismatch.You set icons: 'tweet_splitter' but the button uses icon: 'horizontalrule'. Either add an icon asset or drop the icons property to avoid 404s.
- icons: 'tweet_splitter', + // icons: 'tweet_splitter', // not needed since button uses built-in iconAlso applies to: 38-43
95-107: Bind submit validation to the editor’s owning form, not all forms.Current code attaches to every form on the page and validates all editor instances, which can block unrelated forms. Scope to the editor’s parent form and validate that instance.
Example (move into init after instanceReady):
-document.querySelectorAll("form").forEach(form => { - form.addEventListener("submit", function(e) { - // Берём первый доступный CKEditor (или пробегаем все, если их несколько) - for (let instanceName in CKEDITOR.instances) { - const editor = CKEDITOR.instances[instanceName]; - if (!validateTweets(editor)) { - e.preventDefault(); // блокируем отправку - alert("❌ Нельзя сохранить: один или несколько твитов превышают 280 символов!"); - return false; // выходим из цикла, форма не отправляется - } - } - }); - }); +const container = editor.container && editor.container.$; +const form = container && container.closest ? container.closest('form') : null; +if (form && !form._tweetSplitterBound) { + form._tweetSplitterBound = true; + form.addEventListener('submit', (e) => { + if (!validateTweets(editor)) { + e.preventDefault(); + alert("❌ Нельзя сохранить: один или несколько твитов превышают 280 символов!"); + } + }); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
static/ckeditor/ckeditor/plugins/tweet_splitter/plugin.js(1 hunks)static/ckeditor/ckeditor/plugins/tweet_splitter/styles.css(1 hunks)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
static/ckeditor/ckeditor/plugins/tweet_splitter/styles.css (2)
46-54: Boost contrast/legibility of “Новый твит” label (WCAG).12px uppercase at #999 risks failing contrast for small text. Slightly darken or bump size.
.tweet-separator-text { - padding: 0 15px; - color: #999; - font-size: 12px; + padding: 0 15px; + color: #666; /* better contrast */ + font-size: 13px; font-weight: bold; text-transform: uppercase; display: flex; align-items: center; }
56-66: Increase hit target and add active state for the remove control.Improve usability and keyboard/mouse affordance.
.tweet-separator-remove { cursor: pointer; display: inline-flex; align-items: center; line-height: 1; user-select: none; vertical-align: middle; - margin-left: 5px; - padding: 2px; + margin-left: 5px; + padding: 4px; /* larger tap target */ + min-width: 24px; + min-height: 24px; border-radius: 4px; } .tweet-separator-remove:hover { background-color: #ddd; } +.tweet-separator-remove:active { + background-color: #e3e3e3; +} + .tweet-separator-remove:focus-visible { outline: 2px solid #666; outline-offset: 2px; border-radius: 4px; } .tweet-separator-remove svg { width: 16px; height: 16px; } + +/* pair with JS (see plugin.js) instead of inline red */ +.tweet-char-counter.over-limit { + color: #b3261e; +}Also applies to: 68-76, 78-81
static/ckeditor/ckeditor/plugins/tweet_splitter/plugin.js (5)
129-137: Be robust when detecting separators.Strict equality on className breaks if extra classes are added. Use classList.contains and nodeType guard.
- if (node.className === 'tweet-separator') { + if (node.nodeType === 1 && node.classList && node.classList.contains('tweet-separator')) {Apply in both validateTweets() and updateTweetCounters().
Also applies to: 170-178
57-63: Also debounce updates on paste/drop/undo-redo.Covers more mutation sources beyond key/afterCommandExec.
editor.on('key', function() { scheduleTweetCountersUpdate(editor);; }); editor.on('afterCommandExec', function() { scheduleTweetCountersUpdate(editor);; }); + editor.on('paste', function() { + scheduleTweetCountersUpdate(editor); + }); + editor.on('afterPaste', function() { + scheduleTweetCountersUpdate(editor); + }); + editor.on('drop', function() { + scheduleTweetCountersUpdate(editor); + });
205-207: Avoid inline style for over-limit; use a CSS class.Enables theming and keeps style in CSS.
- if (finalLength > TWEET_LIMIT) { - counter.style.color = 'red'; - } + counter.classList.toggle('over-limit', finalLength > TWEET_LIMIT);Pair with the new
.tweet-char-counter.over-limitrule in styles.css (see related comment).
21-21: Remove stray double semicolons.- scheduleTweetCountersUpdate(editor);; + scheduleTweetCountersUpdate(editor); ... - scheduleTweetCountersUpdate(editor);; + scheduleTweetCountersUpdate(editor); ... - scheduleTweetCountersUpdate(editor);; + scheduleTweetCountersUpdate(editor);Also applies to: 58-58, 62-62
155-164: Optional: avoid getData+regex; compute lengths directly from live DOM.Parsing HTML string is heavier; you already have tweetNodes. Consider deriving textLength from cloned nodes of each tweet and calling twitterLength on their textContent. This reduces serialization cost and keeps DOM/HTML paths consistent.
Also applies to: 184-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
static/ckeditor/ckeditor/plugins/tweet_splitter/plugin.js(1 hunks)static/ckeditor/ckeditor/plugins/tweet_splitter/styles.css(1 hunks)
🔇 Additional comments (3)
static/ckeditor/ckeditor/plugins/tweet_splitter/styles.css (1)
19-29: Avoid float-based counter layout to prevent overlap with content.If the first child is a full-width block, the float may overlap. Consider a non-floated pattern (e.g., flex container wrapper or position: sticky). If you keep float, verify across common CKEditor content blocks.
static/ckeditor/ckeditor/plugins/tweet_splitter/plugin.js (2)
23-43: Event binding may misroute updates with multiple editors sharing the same document.Binding once per nativeDoc and closing over a single editor can desync counters in multi-editor pages.
Prefer per-editor bindings or resolve the editor from the event target:
- Track a per-editor flag: editor._tweetSplitterRemoveBound.
- In the click handler, locate the owning editor by traversing to the closest editor.container and map to CKEDITOR.instances.
If you want, I can draft a per-editor-safe handler.
161-161: Regex for splitting separators looks good.Fix addresses earlier mis-segmentation concern.
|
done |
1 participant
Summary by CodeRabbit
New Features
Style