Skip to content

feat: Modal Selects #2858

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

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

Conversation

NeloBlivion
Copy link
Member

@NeloBlivion NeloBlivion commented Aug 6, 2025

Summary

Upcoming, only works in alpha server; do not merge.
image
image

Information

  • 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, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

@Paillat-dev Paillat-dev self-requested a review August 6, 2025 22:17
Copy link
Contributor

@DA-344 DA-344 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor typo

@Lulalaby Lulalaby requested review from Lulalaby and a team August 6, 2025 23:35
@@ -215,6 +215,7 @@ def __init__(self, *, data: InteractionPayload, state: ConnectionState):
self._from_data(data)

def _from_data(self, data: InteractionPayload):
self._raw_data: InteractionPayload = data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbf we might want to pr that to keep it not just for testing lol

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls

@NeloBlivion
Copy link
Member Author

NeloBlivion commented Aug 7, 2025

Now fully functional:

  • ui.Label does not exist; new attributes and parameters were added to Select and InputText instead
  • Selects in modals require label

Only works in alpha server. Pending docs updates. This should not be merged.

class Label(Component):
"""Represents a Label used in modals as the top-level component.

This is a component that holda another component alongside additional text in modals.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This is a component that holda another component alongside additional text in modals.
This is a component that allows you to add additional text to another component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No plsplspls keep that one lol

@Icebluewolf
Copy link
Member

  • ui.Label does not exist; new attributes and parameters were added to Select and InputText instead

This is what got us into the mess with Action Row though. I think we should implement ui.Label. We can add a helper feature that will wrap it in an empty label if the user does not provide one (if that is even required)

@NeloBlivion
Copy link
Member Author

NeloBlivion commented Aug 7, 2025

  • ui.Label does not exist; new attributes and parameters were added to Select and InputText instead

This is what got us into the mess with Action Row though. I think we should implement ui.Label. We can add a helper feature that will wrap it in an empty label if the user does not provide one (if that is even required)

No, this is an intentional choice to be consistent with my non-breaking approach to CV2. Label will be present in v3 instead.

Lulalaby and others added 6 commits August 8, 2025 00:00
Co-authored-by: DA344 <108473820+DA-344@users.noreply.github.com>
Signed-off-by: Lala Sabathil <aiko@aitsys.dev>
Co-authored-by: Lala Sabathil <aiko@aitsys.dev>
Signed-off-by: UK <41271523+NeloBlivion@users.noreply.github.com>
@@ -428,6 +437,9 @@ def __init__(self, data: SelectMenuPayload):
self.channel_types: list[ChannelType] = [
try_enum(ChannelType, ct) for ct in data.get("channel_types", [])
]
self.required: bool | None = data.get(
"required"
) # Currently defaults to False, pending change
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending change?

Also why Optional[bool] defaulting to None instead of bool defaulting to False?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because required is only for string selects in modals. it doesn't work in message components

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and pending cause advaith is still working on stuff

Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
Signed-off-by: Lala Sabathil <aiko@aitsys.dev>
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.

6 participants