-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Drag and Resize events for workspace comments #8217
Merged
Merged
Conversation
This file contains 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
rachel-fenichel
requested review from
gonfunko
and removed request for
rachel-fenichel
June 18, 2024 19:56
gonfunko
reviewed
Jun 18, 2024
LGTM but somebody officially on the core team will need to approve. |
BeksOmega
approved these changes
Jun 24, 2024
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
The details
Resolves
Fixes: #8113
Unblocks: google/blockly-samples#2330 and issues related to gonfunko/scratch-vm#3
Proposed Changes
Adds CommentDrag and CommentResize events.
The drag event is fired once (with
isStart=false
) when a comment movement drag is started, and again (withisStart=true
) when the drag is finished. The existing CommentMove event will fire immediately afterward. This is consistent with block drag events which fire once at the start and once at the end.The resize event is fired once when a comment resize drag is completed (with
oldSize="$width, $height"
andnewSize="$width, $height"
). This is the first type of event related to resizing as far as I know, although I tried to follow the example of CommentMove and BlockMove.It occurs to me now while writing this that it may be desirable to also fire some kind of event when a resize drag begins. Maybe even the new CommentDrag event could be fired before CommentResize, similarly to how it is fired before CommentMove. The use-case I'm thinking of is google/blockly-samples#2330 where the content highlight fades away when a drag is begun and reappears when a drag is completed. As-is, we could still update the content highlight bounds when a resize drag is completed, but we wouldn't know when the resize drag is started. What do you think?
Reason for Changes
This is generally just a conspicuously missing set of events for workspace comments but we do have some use cases.
#8113 mentions using the new events for real time collaboration. Even in solo mode, this fixes the existing issue that you couldn't undo resizing a comment.
google/blockly-samples#2330 needs these events to hide the content highlight when a drag is started and recompute its bounds when the drag is completed.
gonfunko/scratch-vm#3 mentions needing these events for the Scratch unforking.
Test Coverage
I made unit tests for constructing and serializing the new events. I also manually tested that they are fired when expected. There's still some missing tests for resize events, for the same reason that #4577 is still open.
Documentation
I'm assuming https://developers.google.com/blockly/reference/js/blockly.events_namespace will be automatically updated?
Additional Information
I made a small change that might be considered breaking. In CommentView, size change listeners used to be called continuously while dragging the resize handle, but now are only called once when the drag is completed (so that the new resize event is only fired once). This is analogous to how CommentView handles text changes: it only notifies change listeners when the text input goes out of focus. The rendered comment display is still continuously updated while dragging, but the comment's internal size property is not updated until the end. As far as I know, CommentView and its change listeners are not publicly exported, but it's feasible that users may expect the comment's size property to be continuously updated. What do you think?