-
-
Notifications
You must be signed in to change notification settings - Fork 274
more mouse controlls #2104
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
base: master
Are you sure you want to change the base?
more mouse controlls #2104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it's generally fine syntax-wise, I don't see why or how this can be merged. All of the functionality here is available in native Scratch or through the already existing TurboWarp Blocks extension (e.g., middle mouse button). It would just be a bloat to the gallery
@@ -0,0 +1,137 @@ | |||
// Name: More Mouse Controls | |||
// ID: legobrainbikerMoreMouseControls | |||
// Description: Some features I felt were missing from the mouse, including right click detection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make this description a bit more professional and reminiscent of the extension's qualities?
document.addEventListener("mousewheel", (e) => { | ||
this.mouseWheelDelta = e.deltaY; | ||
Scratch.vm.runtime.startHats( | ||
"legobrainbikerMoreMouseControls_onScroll" | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On scroll is available in native scratch when doing:
when [up/down arrow] pressed
if <not <up/down arrow pressed>> ?:
scroll action
endif
Because the hat block detects scrolling but the key boolean doesn't, so you can detect scrolling like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this, but I thought this would make it easier, especialy when using the other scroll block. also it works with both at the say time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this, but I thought this would make it easier, especialy when using the other scroll block. also it works with both at the say time.
That's a valid point, but it's still probably not worth adding a whole new extension to the gallery though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than that it looks fine but we'll have to check with a moderator to see if this small extension is worth merging since I have no say in the matter
!format |
Duplicate of a lot of the other mouse extension pull requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast review to get something in
this.mouseWheelDeltaX = 0; | ||
this.mouseZoomDelta = 0; | ||
this.buttons = []; | ||
document.addEventListener("wheel", (e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why cant you just use the built in VM api's for this stuff
return this.buttonPressed(args); | ||
} | ||
buttonPressed({ button }) { | ||
console.log(button); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these redundant logs
console.log(this.buttons[Scratch.Cast.toNumber(button)]); | ||
return this.buttons[Scratch.Cast.toNumber(button)] || false; | ||
} | ||
dissableContextMenu() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the way you implement this breaks some addons, and you should allow for reenabling the context menu and scrolling
console.log(this.buttons[Scratch.Cast.toNumber(button)]); | ||
return this.buttons[Scratch.Cast.toNumber(button)] || false; | ||
} | ||
dissableContextMenu() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to check if you already have the event added, otherwise you are just leaking memory..
adds the ability to detect more data from the mouse that would be difficult or imaposible to get otherwise. it's small now, but I'm open to more blocks.