Repository navigation
New addon: "delete other assets" (delete-others) - #7030
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
The functionality works well, I just have some nitpick comments on the code:
- Can you use a single decrementing for loop to delete the costumes? This is a common and recommended method for modifying an array that you are also iterating over.
- Instead of having two separate parts of logic for sound and costume, can you combine them into a unified loop by using ternary operations for the functions that differ.
-- E.gdeleted = (type === "costume") ? target.deleteCostume(0) : target.deleteSound(0); - can you unify the getRestoreCostumeFun and getRestoreSoundFun by making it take "costume" or "sound" as an input and performing the right logic.
The purpose of the unification changes is to reduce code duplication
I'm not saying it needs to be but disable auto save doesn't remove the confirmation but it's still marked as dangerous. |
Samq64
added
type: enhancement
|
@Joeclinton1 done, done and done |
So does it restore all of the deleted costumes/sounds? I think this is quite important and we didn't mention it in #4692. It's sometimes easy to forget about undo. |
|
It sure does! |
It's a browser native confirmation, and it should be dangerous because you will forget to save, here you need to invoke it manually. |
Joeclinton1
left a comment
Joeclinton1
left a comment
There was a problem hiding this comment.
Works well, and code is much cleaner now. Well done!
Samq64
left a comment
Samq64
left a comment
There was a problem hiding this comment.
Could the menu item be made orange by marking it as dangerous? It would also be nice if the restore message was made plural but that isn't really necessary.
I added the dangerous property and set it to true as a test and it seemed to not change the colour? I'm not sure why, perhaps I did something wrong, could someone suggest the code change so that I can see if I'm doing it right?
Yeah that annoyed me too, I'll see if it's decently easy for me to do. |
|
I don't think this should be marked as dangerous, since you can just restore the costumes, there's nothing dangerous about it. This also doesn't remove functionality it adds functionality. |
I think they were talking about changing the "delete others" button in the context menu to use the color orange when hovered over. |
It seems that even with dangerous: true, the button does not change colour when hovered. Tbh, I don't think it needs to be dangerous, there is a confirmation, and you can restore the costumes easily
|
That's a bug in editor dark mode. It does become orange on hover if dark mode is disabled. |
|
Oh ok, I'll add it then |
|
This should be ready for final reviews and approvals |
WorldLanguages
changed the title
delete-others)
Co-authored-by: Maximouse <51849865+mxmou@users.noreply.github.com>
Co-authored-by: Maximouse <51849865+mxmou@users.noreply.github.com>
WorldLanguages
added
the
status: awaiting answer/followup
WorldLanguages
removed
the
status: awaiting answer/followup
* initial commit * remove dem dynamics * confirm * this should work, testing now * here * getting there * DANGER!!!! * fuck yes * no r * Window.addon is crucial to the working of this addon so we will remove it Co-authored-by: Samq64 <81489795+Samq64@users.noreply.github.com> * smaller popup and string changes * reverse * dynam en dis * stupid strings * opti and var name change * shit forgot that -1 * fix sounds being added in thr wrong order * update ver * Update versionAdded Co-authored-by: Maximouse <51849865+mxmou@users.noreply.github.com> * Arrow fix by mxmou Co-authored-by: Maximouse <51849865+mxmou@users.noreply.github.com> * Useless await * Use internal variable instead of abusing Redux * Cleanup variables * Use editingTarget.addCostume instead --------- Co-authored-by: Samq64 <81489795+Samq64@users.noreply.github.com> Co-authored-by: W_L <mundofinky@gmail.com> Co-authored-by: Maximouse <51849865+mxmou@users.noreply.github.com>
7 participants
Resolves #4692
Changes
Adds a button to the right click menu for costumes, backdrops, and sounds to delete all the others.
Reason for changes
Apparently people want this.
Tests
Tested on brave, restore deleted costumes etc work, i think its all good. Please suggest changes to addon.json name and desc etc