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

feat: {slot} placeholder for MenuItem #61

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rexlManu
Copy link

@rexlManu rexlManu commented Jan 23, 2024

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.

@rexlManu rexlManu changed the title feat: {item} placeholder for MenuItem feat: {slot} placeholder for MenuItem Jan 23, 2024
@cj89898
Copy link
Contributor

cj89898 commented Jan 24, 2024

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

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

rexlManu commented Jan 25, 2024

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

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.

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

Successfully merging this pull request may close these issues.

3 participants