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

fix: make insertion markers use new render management system #7307

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Jul 19, 2023

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes N/A

Proposed Changes

Makes is so that insertion markers use the new render management system.

Reason for Changes

Necessary for tearing out the new old render management system.

Test Coverage

Manually tested that insertion markers appear and look correct.

Documentation

N/A

Additional Information

Dependent on #7296

@github-actions github-actions bot added the PR: fix Fixes a bug label Jul 19, 2023
@BeksOmega BeksOmega mentioned this pull request Jul 19, 2023
4 tasks
@BeksOmega BeksOmega marked this pull request as ready for review July 24, 2023 22:05
@BeksOmega BeksOmega requested a review from a team as a code owner July 24, 2023 22:05
@BeksOmega BeksOmega requested a review from maribethb July 24, 2023 22:05
core/workspace_svg.ts Show resolved Hide resolved
// Connect() also renders the insertion marker.
imConn.connect(closest);
}
// Position so that the existing block doesn't move.
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see why imConn has to exist (we throw an error above if it doesn't) but don't see where we're otherwise validating that closest exists. Maybe worth keeping the check? or am i missing where we're checking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

closest is non-nullable! It is always guaranteed to be part of the activeCandidate.

@maribethb
Copy link
Contributor

Reason for Changes
Necessary for tearing out the new render management system.

😱 😛

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 26, 2023
@BeksOmega BeksOmega merged commit c8be2f2 into google:develop Jul 26, 2023
10 checks passed
@BeksOmega BeksOmega deleted the fix/queue-insertion branch May 14, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants