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

No .contextmenu() event callback implemented yet #1217

Closed
Shiuyin opened this issue Sep 17, 2021 · 9 comments
Closed

No .contextmenu() event callback implemented yet #1217

Shiuyin opened this issue Sep 17, 2021 · 9 comments

Comments

@Shiuyin
Copy link

Shiuyin commented Sep 17, 2021

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
Copy link
Member

Fuzzyma commented Sep 18, 2021

Feel free to create PR for that :)

@Fuzzyma Fuzzyma closed this as completed in 2a8b5ea Sep 3, 2023
@Fuzzyma
Copy link
Member

Fuzzyma commented Jun 18, 2024

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
Copy link
Author

Shiuyin commented Jul 16, 2024

@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
Copy link
Member

Fuzzyma commented Jul 16, 2024

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:

const methods = [

@Shiuyin
Copy link
Author

Shiuyin commented Jul 16, 2024

@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
Copy link
Member

Fuzzyma commented Jul 16, 2024

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

@Shiuyin
Copy link
Author

Shiuyin commented Jul 16, 2024

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

@Fuzzyma
Copy link
Member

Fuzzyma commented Jul 16, 2024

@Shiuyin checkout #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
Copy link
Author

Shiuyin commented Jul 17, 2024

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

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

No branches or pull requests

2 participants