svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.01k stars 1.07k forks source link

No .contextmenu() event callback implemented yet #1217

Closed Shiuyin closed 11 months ago

Shiuyin commented 2 years ago

I have searched through both the documentation and the codebase. In neither, there is any result for 'contextmenu'. It's simply missing from the list of supported events (https://svgjs.dev/docs/3.1/events/). Yes, you could differentiate that from the regular .click() callback, but since you support all other events, this one should be supported as well? Or am i missing something?

So instead of:

// SVG.js does not natively have a 'element.contextmenu()' function yet.
this.element.on('contextmenu', (event: Event) => {
...
})

one could write:

this.element.contextmenu((event: Event) => {
...
})
Fuzzyma commented 2 years ago

Feel free to create PR for that :)

Fuzzyma commented 2 months ago

I am sorry, I somehow forgot to publish all the fixes I made. I just released it: https://github.com/svgdotjs/svg.js/releases/tag/3.2.1

Shiuyin commented 1 month ago

@Fuzzyma Sorry to annoy again. While .contextmenu() is now available, all drag and drop events like "dragover", "drop", etc. do not exist yet. According to https://www.w3.org/TR/SVG2/attindex.html#RegularAttributes, drag and drop should be supported.

Fuzzyma commented 1 month ago

It is a mystery to me how people still use the shorthands instead of just using on for all events :D. Feel free to add them to the list:

https://github.com/svgdotjs/svg.js/blob/ec9d2fc952a683aaa9f0c523fbba0786babdf629/src/modules/optional/sugar.js#L163

Shiuyin commented 1 month ago

@Fuzzyma The reason I prefer the shorthand methods is that when using TS, you get more precise types and don't have to add unnecessary type guards. Example:

element.on('dragover', event => onDragOver(event, row, api))

`function onDragOver(event: Event, row: GanttRow, api: GanttApi): void { if (!(event instanceof DragEvent)) return // TODO: Can be removed, once SVG.js supports .dragover()

api.dragOverCb(event, row)

}`

Your .on() functions are only typed to be of type Event, not the more specific DragEvent that fits this case.

Fuzzyma commented 1 month ago

ah i see, maybe we can change the on-typings to be generic

Shiuyin commented 1 month ago

@Fuzzyma That would be even better! I agree that using the generic .on() would be much better, if it emits the correct type!

Fuzzyma commented 1 month ago

@Shiuyin checkout https://github.com/svgdotjs/svg.js/pull/1320 and see if that pr works for you. Give it a good test. Maybe you can break it somehow.

In case you use svg.draw.js: I saw that the plugin breaks the types again so test it without that plugin for now. I will push an update to svg.draw.js later

Shiuyin commented 1 month ago

Sorry for the late reply, will check out by end of the week and report back. Thank you for your efforts! :)