Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#4575 - New Sequences are created while we add cyclic to middle of sequence. #4703

Conversation

Guch1g0v
Copy link
Collaborator

@Guch1g0v Guch1g0v commented May 28, 2024

Check list

  • unit-tests written
  • e2e-tests written
  • documentation updated
  • PR name follows the pattern #1234 – issue name
  • branch name doesn't contain '#'
  • PR is linked with the issue
  • base branch (master or release/xx) is correct
  • task status changed to "Code review"
  • reviewers are notified about the pull request

@Guch1g0v Guch1g0v linked an issue May 28, 2024 that may be closed by this pull request
@Guch1g0v Guch1g0v changed the title #4575 - Fix: prevent creation of new sequences when adding cyclic to … #4575 - New Sequences are created while we add cyclic to middle of sequence. May 28, 2024
@Guch1g0v Guch1g0v marked this pull request as ready for review May 30, 2024 17:47
Copy link
Collaborator

@svvald svvald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, no critical, can be addressed later to avoid blocking into upcoming release. Overall looks good to me

@@ -883,6 +941,14 @@ export class SequenceMode extends BaseMode {
return modelChanges;
}

private showMergeWarningModal(editor: CoreEditor) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's rather an error than a warning message (which I covered by a pop-up in the bottom of the editor but doesn't block it)

);
}
if (isPasteInStart) {
return this.isR2Free(lastNodeOfNewFragment) && this.isR1Free(currentNode);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need to check both R1 and R2 when pasting into start or end of sequence, you may probably use areR1R2Free here rather than separate functions

@svvald svvald merged commit eb9de63 into master May 31, 2024
5 checks passed
@svvald svvald deleted the 4575-new-sequences-are-created-while-we-add-cyclic-to-middle-of-sequence branch May 31, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants