HelpChat / DeluxeMenus

DeluxeMenus but open source!?
https://wiki.helpch.at/clips-plugins/deluxemenus
MIT License
72 stars 35 forks source link

feat: `{slot}` placeholder for MenuItem #61

Open rexlManu opened 7 months ago

rexlManu commented 7 months ago

Hello,

this PR adds the feature that you have access to the current slot of the menu item via the {slot} placeholder when building dynamic inventories with filler items.

I added a new parameter to the setPlaceholders Method. The parameter slot is filled with the value of the MenuItemOptions#slot(). In places where it doesn't make sense like opening and closing click handlers, I decided to give -1 as value. This is also the case for ClickAction via the execute command.

So the value -1 means that it Item doesn't have anything todo with a MenuItem directly.

This was commissioned work by makiil.grosnach.

cj89898 commented 7 months ago

Wow, great job! So far from what I've seen, it works really, really well! I tried doing this a while back, but failed miserably 😂

I just tested it out with my DeluxeShop plugin and it works flawlessly. My entire shop is just 1 item!

QIA1Ik6

Reference: #25

BlitzOffline commented 7 months ago

At some point, "internal" placeholders were discussed, but nothing came of it. One concern that remains from that conversation is backward compatibility. The concern stems from the fact that the {slot} placeholder could be overriding any already existent argument called {slot}.

I am going to spend some time putting together a POC for internal placeholders to see if we can maybe merge this into that.

rexlManu commented 6 months ago

At some point, "internal" placeholders were discussed, but nothing came of it. One concern that remains from that conversation is backward compatibility. The concern stems from the fact that the {slot} placeholder could be overriding any already existent argument called {slot}.

I am going to spend some time putting together a POC for internal placeholders to see if we can maybe merge this into that.

The only solution to that would be to use a different symbol instead of {}?

BlitzOffline commented 6 months ago

The only solution to that would be to use a different symbol instead of {}?

This is what I have in mind. The problem is that there aren't many good ones we can use. I am not a fan of %% which PlaceholderAPI uses because you can't tell when the placeholder is supposed to start or end. Using ${} might create confusion since it is pretty similar to arguments. I think the best one we can choose now is <> but this can be discussed further.

One other mention I have is that we might want to implement a proper "internal" placeholder system instead of just adding more parameters to the parse function whenever we want new placeholders.

We can probably build upon this PR since it seems to work fine but we have to remember, that data is not always available. For example, we can't have the slot number in the menu title since the title is parsed only once. So we would have context-dependent placeholders.