fix: improve performance of connecting blocks#6876
Merged
BeksOmega merged 11 commits intoRaspberryPiFoundation:developfrom Mar 16, 2023
Merged
fix: improve performance of connecting blocks#6876BeksOmega merged 11 commits intoRaspberryPiFoundation:developfrom
BeksOmega merged 11 commits intoRaspberryPiFoundation:developfrom
Conversation
Improve INP by allowing the browser to do a paint (closing the context menu) before we trigger callbacks. This improves the user experience for expensive callbacks (e.g. collapsing, or updating disabled).
BeksOmega
force-pushed
the
fix/wrapping-performance
branch
from
6349740 to
99b95a4
Compare
When disconnecting the last block in the stack, the block would not be rerendered correctly (the top-start corner would not be reshaped)
The order for applying connections was changed so that connections were applied and then the insertion marker was hidden. This caused an error because hiding the insertion marker expected there to be a child block when there was not.
BeksOmega
commented
Mar 8, 2023
Contributor
Author
|
@NeilFraser This is ready for a look! Might be advisable to review this commit-wise. |
| if (!parentConnection || !childConnection) { | ||
| throw Error('Source connection not connected.'); | ||
| } | ||
| if (otherConnection.targetConnection !== this) { |
Collaborator
There was a problem hiding this comment.
This used to double-check that the connections actually point at each other. Why did you remove that check?
Contributor
Author
There was a problem hiding this comment.
It seems a bit silly when we have the connectReciprocally method. If this check errors, afaict it just means we coded a bug (or someone messed with targetConnection directly, which is also our failing for making that public). Imo, these assertions should be covered by unit tests, not code. But I may be missing some context here.
This was referenced Mar 14, 2023
rachel-fenichel
approved these changes
Mar 16, 2023
cpcallen
reviewed
Mar 17, 2023
Collaborator
There was a problem hiding this comment.
ConnectionType is now unused, and you missed the lint warning (but now we can all enjoy it! 🤣)
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The basics
npm run formatandnpm run lintThe details
Resolves
Related to #6130
Proposed Changes
Optimizes the performance of connecting blocks by:
updateDisabledReason for Changes
Cuts off ~100ms from wrapping an if block around the spaghetti code test blocks.
Test Coverage
Only manual testing :/
Documentation
N/A
Additional Information
Dependent on #6860