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

Erase all button on menu #152

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

topninja
Copy link

this is for #151 #120
add erase all button on menu

@@ -90,6 +90,7 @@
<script src="../js/minitpl.js"></script>
<script src="../js/board.js"></script>
<script src="../tools/pencil/wbo_pencil_point.js"></script>
<script src="../tools/new/new.js"></script>
Copy link
Owner

Choose a reason for hiding this comment

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

It should probably not be called new, but something like clear

Comment on lines +30 to +37
Tools.socket.emit('broadcast', {
board: Tools.boardName,
data : {
tool : "New",
type : "deleteall"
}
});
Tools.drawingArea.innerHTML = '';
Copy link
Owner

Choose a reason for hiding this comment

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

Tools.socket shouldn't be used directly. You can use drawAndSend. See other tools for an example.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, there should probably be a confirmation prompt. This erases all data forever, which could represent days of work by multiple people.

Comment on lines +43 to +45
case "deleteall":
Tools.drawingArea.innerHTML = '';
break;
Copy link
Owner

Choose a reason for hiding this comment

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

You have a formatting problem

}

Tools.add({
"name": "New",
Copy link
Owner

Choose a reason for hiding this comment

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

it shouldn't be called new

"name": "New",
"shortcut": "n",
"listeners": {},
"icon": "tools/new/new.svg",
Copy link
Owner

Choose a reason for hiding this comment

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

not new

Comment on lines +1 to +9
<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 24.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0) -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
viewBox="0 0 512 512" style="enable-background:new 0 0 512 512;" xml:space="preserve">
<g>
<polygon points="448,80 512,80 512,112 448,112 448,176 416,176 416,112 352,112 352,80 416,80 416,16 448,16 "/>
<path d="M32,112h256V80H32C14.3,80,0,94.3,0,112v352c0,17.7,14.3,32,32,32h384c17.7,0,32-14.3,32-32V247.3h-32V464H32V112z"/>
</g>
</svg>
Copy link
Owner

Choose a reason for hiding this comment

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

This icon makes it look like something new is being created, wheras all the data on the board is erased and lost forever.

*/
BoardData.prototype.deleteall = function () {
//KISS
this.board = [];
Copy link
Owner

Choose a reason for hiding this comment

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

This.board is an object, not an array

@@ -143,6 +143,9 @@ async function saveHistory(boardName, message) {
case "delete":
if (id) board.delete(id);
break;
case "deleteall":
board.deleteall();
Copy link
Owner

Choose a reason for hiding this comment

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

We should check that the tool is activated, and this tool should probably not be activated by default.

@lovasoa
Copy link
Owner

lovasoa commented Nov 27, 2020

See #92 and #90 and the discussion in jitsi/jitsi-meet#5295

Such a tool is very dangerous, since it allows a single misbehaving user to sabotage the work of all others on a large whiteboard. This tool should be disabled by default, and be possible to enable through configuration.

@topninja
Copy link
Author

I agree with your suggesstion @lovasoa

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.

3 participants