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!: change paste to return the pasted thing to support keyboard nav #5996

Merged
merged 5 commits into from
Mar 15, 2022

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Mar 14, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

Work on google/blockly-samples#833

Proposed Changes

Changes the paste functions on the clipboard and the workspace to return the pasted thing (block or workspace comment).

Reason for Changes

Keyboard nav needs to grab the pasted thing to then move it to the correct spot. This gives keyboard nav a simple and safe API for doing so.

Test Coverage

N/A

Documentation

N/A

Additional Information

I'm not sure if this is the thing we want to do or not, because it is a breaking change for anyone using clipboard.paste(thing) === true. I just thought posting a PR would be the best way to discuss.

(Personally I think this is the best API, but totally willing to change it for backwards compat reasons)

@BeksOmega BeksOmega requested a review from a team as a code owner March 14, 2022 21:32
@BeksOmega BeksOmega requested a review from gonfunko March 14, 2022 21:32
Copy link
Contributor

@gonfunko gonfunko left a comment

Choose a reason for hiding this comment

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

This looks good to me; I like the general approach and tend to agree it seems unlikely the API change would be problematic. Probably worth checking with the rest of the team to confirm folks are good with it though before submitting.

@@ -38,13 +42,14 @@ exports.copy = copy;

/**
* Paste a block or workspace comment on to the main workspace.
* @return {boolean} True if the paste was successful, false otherwise.
* @return {!BlockSvg|!WorkspaceCommentSvg|null} The pasted thing if the paste
Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably be cleaner and more future-proof if you used ICopyable as the return type like in duplicate(). Probably want to do this in workspace_svg as well.

@BeksOmega BeksOmega merged commit 20f1475 into google:develop Mar 15, 2022
@BeksOmega BeksOmega deleted the fix/keyboard-nav branch April 5, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants