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.
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
Zoom out: Try vertical displacement when dragging a pattern between existing patterns/sections #63896
Zoom out: Try vertical displacement when dragging a pattern between existing patterns/sections #63896
Changes from all commits
8ec69c6
f67d2be
63586db
69fd429
229c2ae
2be4551
692e511
6ed6562
f35ae44
d87b7d6
e8a0ab8
dfcc570
55fc09e
bdcb6a4
94a8a14
7ab2a92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use the new selector here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are too many of these conditions creeping in which disallow non-"insert" events in Zoom Out mode. Whilst this is desired behaviour I'd like to codify the concept and have it made reusable. One for a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now would it work to move this check up to the existing
isZoomOutMode()
check earlier in this code block so that the zoomed out mode checks are consolidated?Leaving to follow-ups is also a valid approach! My (very) weakly held opinion here is that sometimes it can be tricky to change some of these rules after the fact if we leave them for a while as the drag and drop logic can be a bit challenging to reason about — I know I struggle looking at some of my past code changes to this area of the codebase 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to do that but the issue is that
operation
isn't available until this point. That in turn is dependent on other steps above. We could bring all the ZoomOut checks down to this location, but then we'd be doing unnecessary work rather than existing early.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we need a code quality PR to track down how many instances of ignoring non "insert" operations in Zoom Out mode exist in the code. Once we know this we can decide whether or not to refactor. It might be premature at this stage if there are only a couple.