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(modal, embeds): new method for Embed and metaclass for class Modal #1254

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shayzi3
Copy link

@shayzi3 shayzi3 commented Dec 10, 2024

Summary

Embed

New method Embed.add_some_fields in order to be able to create multiple fields at once without using add_field multiple times.

For example,

user_profile = get_profile(user_id) # -> List[Dict]

emb = Embed(title="Profile")
emb.add_some_fields(*user_profile)

Modal

I also think that you need to create a Modal using metaclass. This improves readability.

Example

import disnake

class MyModal(disnake.ui.Modal):
    __title__ = "Create Tag"
    __custom_id__ = "create_tag"
    
    name = disnake.ui.TextInput(
        label="Name",
        placeholder="The name of the tag",
        custom_id="name",
        style=disnake.TextInputStyle.short,
        min_length=5,
        max_length=50,
    )
    content = disnake.ui.TextInput(
        label="Content",
        placeholder="The content of the tag",
        custom_id="content",
        style=disnake.TextInputStyle.paragraph,
        min_length=5,
        max_length=1024,
    )

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@Snipy7374
Copy link
Collaborator

If you really want to add multiple fields at once you could use disnake.Embed.from_dict. Beside that, I don't really see a point on doing it.

The proposed change for modals is absolutely a breaking change, you should first try to discuss this with maintainers before making a PR.

@shiftinv
Copy link
Member

Thanks for the PR! First off, please try to keep PRs limited to one feature/fix at a time, otherwise it makes maintaining/reviewing much more difficult. Feel free to open as many as you'd like, though. :)

That said, regarding the two additions:

embeds

I agree with @Snipy7374 that a method like Embed.add_some_fields is unnecessary.
from_dict exists, and implementing your own wrapper around add_field (perhaps even by subclassing Embed) would be fairly simple too.

modals

This is a breaking change, but a reasonable idea overall. Components in general will undergo a significant refactoring in the next major version (3.0), so it might make sense to bundle this feature with those changes, since breaking changes are inevitable there. Would have to put this one the back burner until then, though.
At first glance, I can see a couple issues:

  • Ideally, both the old and new functionality of initializing a modal would be supported. I don't know if it's possible to enforce this correctly on the typing level, though.
  • A metaclass is somewhat overkill, __init_subclass__ is fine for these purposes.
  • Since modal inputs would then be attributes of modals, there will likely be confusion about modal.<input>.value, which doesn't reflect the submitted value (but instead the pre-filled value sent to the API). This isn't too big of a deal.

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