8000 fix: drag strategy only clear group id set by us by HollowMan6 · Pull Request #8355 · google/blockly · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: drag strategy only clear group id set by us #8355

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

Merged
merged 1 commit into from
Jul 25, 2024
Merged

fix: drag strategy only clear group id set by us #8355

merged 1 commit into from
Jul 25, 2024

Conversation

HollowMan6
Copy link
Contributor

The basics

The details

Resolves

Fixes mit-cml/workspace-multiselect#62 (comment)

Proposed Changes

We should add a condition check so that we don't unset the group ID that is not set by us, otherwise, the multi-select plugin undo/redo will be broken (apply individually instead of all together)

Reason for Changes

If not, then a monkey patch will be needed on the multi-selection plugin side.

Test Coverage

Documentation

Additional Information

@HollowMan6 HollowMan6 requested a review from a team as a code owner July 13, 2024 08:47
@HollowMan6 HollowMan6 requested a review from cpcallen July 13, 2024 08:47
@github-actions github-actions bot added the PR: fix Fixes a bug label Jul 13, 2024
Copy link
Contributor
@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Hello HolowMan6, and thanks for this PR. This seems like a sensible fix, and the code looks great—I have only a couple of minor nits re: documentation and organisation.

@HollowMan6
Copy link
Contributor Author

Hi @cpcallen! Thanks for reviewing, I just updated the code according to your change request!

@HollowMan6 HollowMan6 requested a review from cpcallen July 17, 2024 18:37
@cpcallen
Copy link
Contributor

@HollowMan6: test failures are due to issues with node.js v22.5.0 (see #8392 and #8393). Can you rebase this PR onto latest develop, please?

We should add a condition check so that we don't unset
the group ID that is not set by us, otherwise, the
multi-select plugin undo/redo will be broken (apply
individually instead of all together)

Signed-off-by: Hollow Man <hollowman@opensuse.org>
@HollowMan6
Copy link
Contributor Author

@HollowMan6: test failures are due to issues with node.js v22.5.0 (see #8392 and #8393). Can you rebase this PR onto latest develop, please?

Done!

@HollowMan6 HollowMan6 requested a review from cpcallen July 23, 2024 12:28
@cpcallen cpcallen merged commit 504de6a into google:develop Jul 25, 2024
8 checks passed
@RoboErikG
Copy link
Contributor

Hi @HollowMan6 and @ewpatton

This change has caused some issues with ending event groups when you drag a block out of the toolbox (#8764). I'm not very familiar with the multi-select plugin, so I'm not sure what options there are for an alternative fix. Would you like to take a look before I revert this change and file an issue to follow up on the plugin?

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 3AAD .

3 participants
0