fix paste with new line #2696
fix paste with new line #2696
Conversation
Reviewer's Guide by SourceryThis PR modifies the line filtering behavior in the x-input component to properly handle pasted text containing newlines. Instead of completely blocking multi-line input when pasting, it now removes newline characters while preserving the content. Sequence diagram for paste handling in x-input componentsequenceDiagram
participant User
participant XInputComponent
participant EditorState
User->>XInputComponent: Paste text
XInputComponent->>EditorState: Create transaction
EditorState->>XInputComponent: Transaction with changes
XInputComponent->>XInputComponent: Check if paste event
alt Paste event with new lines
XInputComponent->>XInputComponent: Remove newlines
else No new lines or not paste event
XInputComponent->>XInputComponent: Allow transaction
end
XInputComponent->>EditorState: Apply changes
EditorState->>User: Updated text without newlines
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @imolorhe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please complete the PR checklist (running tests, updating documentation, etc) and mark the items when done.
- Could you expand the PR description to explain the specific changes made and how they fix the paste behavior?
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -126,8 +132,28 @@ export class XInputComponent implements AfterViewInit, ControlValueAccessor { | |||
| }, | |||
| }, | |||
| }); | |||
|
|
|||
| const filterNewLine = EditorState.transactionFilter.of((tr) => { | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the newline filtering logic by using string replacement instead of iterative change tracking
The current implementation is unnecessarily complex with nested iterations and change tracking. Here's a simpler approach that maintains the same functionality:
const filterNewLine = EditorState.transactionFilter.of((tr) => {
if (tr.changes.empty) return tr;
if (tr.isUserEvent('input.paste')) {
// For paste events, replace newlines with spaces
const changes = [{
from: 0,
to: tr.newDoc.length,
insert: tr.newDoc.toString().replace(/\n/g, ' ')
}];
return [{ changes }];
}
// Block multi-line input from other sources
return tr.newDoc.lines > 1 ? [] : [tr];
});This achieves the same result more clearly by:
- Handling paste events with a simple string replacement
- Blocking other multi-line input directly
- Eliminating the need for nested iterations and change tracking
There was a problem hiding this comment.
That's brilliant! It's a much simpler logic to the problem. I only had to tweak a bit but most of the logic worked!
| const filterNewLine = EditorState.transactionFilter.of((tr) => { | ||
| return tr.newDoc.lines > 1 ? [] : [tr]; | ||
| if (tr.changes.empty) return tr; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (tr.changes.empty) return tr; | |
| if (tr.changes.empty) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| const removeNLs: ChangeSpec[] = []; | ||
| tr.changes.iterChanges((fromA, toA, fromB, toB, ins) => { | ||
| const lineIter = ins.iterLines().next(); | ||
| if (ins.lines <= 1) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (ins.lines <= 1) return; | |
| if (ins.lines <= 1) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
Visit the preview URL for this PR (updated for commit 22188f2): https://altair-gql--pr2696-imolorhe-fix-paste-w-lcn91wnm.web.app (expires Thu, 14 Nov 2024 10:22:41 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
Fixes
Closes #2681
Checks
yarn test-buildChanges proposed in this pull request:
Summary by Sourcery
Bug Fixes: