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

Add bulk delete feature #3885

Merged
merged 7 commits into from
May 22, 2024
Merged

Conversation

apsinghdev
Copy link
Contributor

fixes #3840

This PR adds a bulk delete feature. It lets a user drag to select blocks and delete them in a single click.

Here is the demo of this

2024-05-04.23-37-46.mp4

@apsinghdev
Copy link
Contributor Author

apsinghdev commented May 4, 2024

@walterbender please take a look and suggest modifications if any.

@walterbender
Copy link
Member

In general, it seems to work well. However, there is one critical bug (apparent in my testing and in your video). If you delete a Note Block, it seems to be creating a Silence Block. I suspect that this is due to the order in which the blocks are being deleted. If you first delete the Pitch Block inside of a Note Block through the normal delete mechanism, it will create a Silence Block. That Silence Block is not on the bulk-delete list. So I think you may need to delete from the outside-in rather than in either an arbitrary order or inside out.

One small issue with coding style: please be consistent with the use of spaces around operators.

Also, it seems that restore will restore one block at a time, which I guess is OK for now. At some point, we may consider restoring the entire bulk delete at once. But I would save that for a different MR.

@apsinghdev
Copy link
Contributor Author

@walterbender sir Thanks for the review. I've got your point about getting the silence block after deleting the blocks. I will fix this bug as well as the coding style soon. Also, after completing this issue, I'll go ahead and implement Bulk-Restore as well.

@apsinghdev
Copy link
Contributor Author

apsinghdev commented May 12, 2024

@walterbender can you please elaborate on this line "you may need to delete from the outside-in rather than in either an arbitrary order or inside out." a bit more?

I did some digging in the codebase and found the currently used delete mechanism to delete blocks. are there other better methods available in the codebase to delete the blocks?

@walterbender
Copy link
Member

walterbender commented May 12, 2024 via email

@apsinghdev
Copy link
Contributor Author

@walterbender I have made the changes and the feature seems working fine. Please take a look.

2024-05-22.23-10-44.mp4

js/block.js Outdated Show resolved Hide resolved
js/block.js Outdated Show resolved Hide resolved
js/block.js Outdated Show resolved Hide resolved
js/activity.js Outdated Show resolved Hide resolved
js/activity.js Outdated Show resolved Hide resolved
js/activity.js Outdated Show resolved Hide resolved
js/activity.js Outdated Show resolved Hide resolved
@walterbender walterbender merged commit 5413a5b into sugarlabs:master May 22, 2024
3 checks passed
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.

Add a bulk delete feature
2 participants