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

Crearting 'View Trash' and more restore option in Musicblocks #4191

Merged
merged 12 commits into from
Jan 18, 2025

Conversation

subhas-pramanik-09
Copy link
Contributor

resolves #4190

I have created a new Restore option in Musicblocks ( View Trash ). From here we can see the list of all trashed items. By selecting item from the list the specific item will be restored. We don't need to restore item multiple time to restore the specific item from trash.

Screen Record

Screen.Recording.2024-12-27.175506.mp4

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender Sir, I think it will definitely help users. Please check it.

Copy link
Contributor

@AliyanA1 AliyanA1 left a comment

Choose a reason for hiding this comment

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

This is good

@walterbender
Copy link
Member

I like the basic idea (we have something similiar in the Python version of Turtle Blocks). But I am loath to add yet another button to the toolbar. Maybe this is the default behavior of the restore button? @pikurasa what do you think?

@subhas-pramanik-09
Copy link
Contributor Author

I like the basic idea (we have something similiar in the Python version of Turtle Blocks). But I am loath to add yet another button to the toolbar. Maybe this is the default behavior of the restore button? @pikurasa what do you think?

Sir which should be the default behavior previous functionality or the functionality in this PR ?

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender @pikurasa Sir can we add this feature to the current Restore button so we don't need to create a new button in toolbar again.

@walterbender
Copy link
Member

Let's wait to hear from @pikurasa

@pikurasa
Copy link
Collaborator

I think that the implementation in the python Turtle Art is better than this, and I think, if we do it at all, we should do it more similar to that.

I think the design should be:

  • There is a single trash button.
  • When you click the trash button, you see all the items that are in the trash bin, like you can see items in a palette.
  • Unlike the palette, you get some special buttons, such as "restore last" and "restore all". These buttons should probably be sticky at the top of the palette.
  • "Restore last" behaves the way our current "Restore" works, and "restore all" restores all the items in the trash.
  • (For now, we can just implement "restore last". We can implement "restore all" later. And we can discuss whether or not we want to be able to "empty trash" as you can with Turtle Art.)

@subhas-pramanik-09
Copy link
Contributor Author

Okay I am working on it.

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender @pikurasa Sir, I have changed it like this. If any changes required please let me know

Screen.Recording.2024-12-31.004222.mp4

@pikurasa
Copy link
Collaborator

@walterbender @pikurasa Sir, I have changed it like this. If any changes required please let me know
Screen.Recording.2024-12-31.004222.mp4

I like that design, in general.

The blocks showing as black and white; I can't decide on whether that's a good thing or not. It may be nice that they don't render in color, as that kind of indicates that they are " currently trashed".

@walterbender
Copy link
Member

Looks good. I think the only change would be to put the buttons all the way to the top so the blocks don't appear above the buttons.

@subhas-pramanik-09
Copy link
Contributor Author

Now this looks like

Screenshot 2024-12-31 093708

@walterbender
Copy link
Member

If I understand it correctly, the button on the piemenu pops one item off the trash stack, while the toolbar button displays the trash list? I think that is a good decision.

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
trashView.id = 'trashView';
trashView.classList.add('trash-view');

trashView.style = `position: relative; background-color: white; max-width: 396px; max-height: 200px;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should move this to the CSS file?

js/activity.js Outdated

// Sticky buttons
const buttonContainer = document.createElement('div');
buttonContainer.style = `position: sticky; top: 0; z-index: 10; display: flex; gap: 10px; background: white;
Copy link
Member

Choose a reason for hiding this comment

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

And these other style definitions?

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender Sir I have changed according to you said. Please review

@walterbender
Copy link
Member

For some reason, the button label is not appearing.
Screenshot From 2024-12-31 17-27-06

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender Sir I have tested in my local machine and different browsers also it works as expected.
Screenshot 2025-01-01 100041
Screenshot 2025-01-01 100241

@walterbender
Copy link
Member

I will try again. I am using the latest Firefox on Fedora.

@walterbender
Copy link
Member

I don't see anything in the logs.

text-align: left;
}

.button-container {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the background for this container should be the same blue we use for toolbars.
Maybe the buttons should be icons with hover help text instead of labels. (We'll need to decide on what icons.)

js/activity.js Outdated
buttonContainer.classList.add('button-container');

const restoreLastBtn = document.createElement('button');
restoreLastBtn.textContent = 'Restore Last';
Copy link
Member

Choose a reason for hiding this comment

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

This string needs i18n. _("Restore Last")

Same for Restore All on Line 3477.

Also, we should use "Restore last" and "Restore all" to be consistent with the other menu labels.

@subhas-pramanik-09
Copy link
Contributor Author

Sir I have tested in a different machine with latest firefox but it works...I am trying to find

@subhas-pramanik-09
Copy link
Contributor Author

I kept it buttons for now if we need icons i will change it later on.

Screenshot 2025-01-01 184208

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender @pikurasa Sir, what should we kept it button or icon ?

@walterbender
Copy link
Member

I would prefer icons with hover help as it is consistent with the rest of the interface,

@subhas-pramanik-09
Copy link
Contributor Author

I would prefer icons with hover help as it is consistent with the rest of the interface,

Which icon should I use ?

@walterbender
Copy link
Member

I was thinking of something like these...
restore_all
restore_last

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender Sir I am unable to find these icons which you sent. But I made the changes with these icons

image

@walterbender
Copy link
Member

+1 to using icons.
I think we may want to generate our own icons though... but we can do that in a separate PR.

@walterbender
Copy link
Member

But the icons should have some hover help strings.

@subhas-pramanik-09
Copy link
Contributor Author

But the icons should have some hover help strings.

Yes Sir I am trying to do that. Can you help me to find out the related code for it ?

@walterbender
Copy link
Member

There are lots of examples of tooltips in js/toolbar.js

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender Sir now icons have hover help strings like

image

js/activity.js Outdated Show resolved Hide resolved
js/activity.js Outdated Show resolved Hide resolved
@subhas-pramanik-09
Copy link
Contributor Author

Yes Sir I have changed it. Please check.

@subhas-pramanik-09 subhas-pramanik-09 changed the title Crearting 'View Trash' option in Musicblocks Crearting 'View Trash' and more restore option in Musicblocks Jan 16, 2025
@walterbender
Copy link
Member

I was doing one more round of testing and bumped into a problem: the restore last item button doesn't seem to restore the last item if there is only one item in the trash.

Steps to reproduce:

  1. delete something; there should be one item in the trash.
  2. delete something else; there should be two items in the trash.
  3. restore one item (either by clicking on it or using the restore last item button); there should be one item in the trash.
  4. try using the restore last item button.
  5. observe that nothing is restored and there is still one item in the trash.

Both the restore all items button and clicking on the item will restore the last item.

@subhas-pramanik-09
Copy link
Contributor Author

I was doing one more round of testing and bumped into a problem: the restore last item button doesn't seem to restore the last item if there is only one item in the trash.

Steps to reproduce:

  1. delete something; there should be one item in the trash.
  2. delete something else; there should be two items in the trash.
  3. restore one item (either by clicking on it or using the restore last item button); there should be one item in the trash.
  4. try using the restore last item button.
  5. observe that nothing is restored and there is still one item in the trash.

Both the restore all items button and clicking on the item will restore the last item.

Sir I reproduced this so many times. But it works properly like it

Screen.Recording.2025-01-16.211209.mp4

@walterbender
Copy link
Member

I was using Firefox. Maybe something browser related? But it seems unlikely.

@subhas-pramanik-09
Copy link
Contributor Author

I was using Firefox. Maybe something browser related? But it seems unlikely.

I recorded this video in Firefox. Before that i used Chrome and Brave. Sir could you please check it once again ?

@walterbender
Copy link
Member

I think I figured out the specifics of when it breaks.
If the last block to be restored is a Start block, the restore last item doesn't seem to work. Otherwise it seems to work. Can you please test that condition?

@subhas-pramanik-09
Copy link
Contributor Author

I think I figured out the specifics of when it breaks. If the last block to be restored is a Start block, the restore last item doesn't seem to work. Otherwise it seems to work. Can you please test that condition?

@walterbender Yes Sir. In this case Restore last item was not working. But after this change it is working as expected.

Screen.Recording.2025-01-17.110618.mp4

js/activity.js Outdated
restoreLastIcon.classList.add('restore-last-icon');
restoreLastIcon.innerHTML = '<i class="material-icons md-48">restore_from_trash</i>';
restoreLastIcon.addEventListener('click', () => {
for(let i=this.blocks.trashStacks.length - 1;i<this.blocks.trashStacks.length;i++){
Copy link
Member

Choose a reason for hiding this comment

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

This seems overly complicated. Why do it this way?
Also, please be sure to include spaces around operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems overly complicated. Why do it this way? Also, please be sure to include spaces around operators.

Yes this way is little bit complicated. I tried to handle the edge case many different ways but it not works. But it works for restore all because it uses loop. So to handle this case I switched to loop so that functionality remains constant for all cases.

@subhas-pramanik-09
Copy link
Contributor Author

Okay. I added spaces around operators.

@subhas-pramanik-09
Copy link
Contributor Author

@walterbender Sir there any changes required ?

@walterbender
Copy link
Member

You tested:
this._restoreTrashById(this.blocks.trashStacks[this.blocks.trashStacks.length - 1]);

I don't know why that couldn't replace the for loop.

@subhas-pramanik-09
Copy link
Contributor Author

You tested: this._restoreTrashById(this.blocks.trashStacks[this.blocks.trashStacks.length - 1]);

I don't know why that couldn't replace the for loop.

Sir I replace the loop with this line now it also works for start block in every condition. Could you please check it once ?

Screen.Recording.2025-01-18.215901.mp4

@walterbender walterbender merged commit 39171f4 into sugarlabs:master Jan 18, 2025
4 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.

Enhancement of restore items option in Musicblocks
4 participants