search

KageJittai / lets-trade-5e

A Foundry VTT module which allows you to send and receive items from other players.
MIT License
11 stars 14 forks source link

Add api for module interaction for solve https://github.com/KageJittai/lets-trade-5e/issues/41 #42

Closed p4535992 closed 1 year ago

p4535992 commented 1 year ago

Add api for module interaction for solve https://github.com/KageJittai/lets-trade-5e/issues/41

game.modules.get("lets-trade-5e").api.openCurrencyTrade(actorId) => void

game.modules.get("lets-trade-5e").api.openItemTrade(actorId, itemId) => void

szefo09 commented 1 year ago

I'm not sure why this is needed, when it can be resolved like proposed in #41 instead of doing Tidy5e hacks This way only Tidy5e Sheet will have additional button in context menu because it implements the API, instead of all sheets that have context menu benefiting from it.

KageJittai commented 1 year ago

I'm aimed to agree with @szefo09 that this isn't required. I'm willing to be convinced otherwise if you can provide a good rational for opening this up.

p4535992 commented 1 year ago

For the current use case (the menu context one) I don't particularly care what solution you want to adopt it's perfectly fine to use the hook as well, I had done the API solution because I didn't know how much and when you wanted to redo the module and it was a minimal effort solution on your side without rewriting anything of the current code.

An api for the module I would put in regardless for the following reasons:

1) Useful at the macro level and/or otherwise for integration with other modules. 2) As far as Tidye5 Sheet is concerned I have to work a lot with css and if I change the name of the specific css class I break the integration with your module (as was pointed out to me in a recent issue), with an api I don't have this problem (i'll manage the integration on my side without bother you )

KageJittai commented 1 year ago

Ok, that's fine with me if TidySheets wants this for integration purposes. I have some other changes I need, but planning on doing a build today or tomorrow so I'll include this PR with those other changes.