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: drag strategy only clear group id set by us #8355

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.

core/dragging/block_drag_strategy.ts Outdated Show resolved Hide resolved
@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
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.

2 participants