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(util): add component builders #1752

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

itohatweb
Copy link
Member

No description provided.

@github-actions github-actions bot added c-util Affects the util crate c-validate Affects the validate crate t-feature Addition of a new feature labels May 29, 2022
@zeylahellyer zeylahellyer marked this pull request as draft May 30, 2022 05:27
@itohatweb itohatweb marked this pull request as ready for review June 1, 2022 13:46
Comment on lines +142 to +143
pub fn components(mut self, components: &mut Vec<Component>) -> Self {
self.0.components.append(components);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are taking ownership of the items in the Vec anyway, we could reborrow it for a cleaner signature:

Suggested change
pub fn components(mut self, components: &mut Vec<Component>) -> Self {
self.0.components.append(components);
pub fn components(mut self, components: Vec<Component>) -> Self {
let mut components = components;
self.0.components.append(&mut components);

I think this can be applied throughout the PR if it's the case

//! # Ok(()) }
//! ```

use std::convert::TryFrom;
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is in the prelude now? it's in a couple other files too

Comment on lines +238 to +240
/// Convert a components builder into a `Vec<Components>`, validating its contents.
///
/// This is equivalent to calling [`Components::validate`], then
Copy link
Contributor

Choose a reason for hiding this comment

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

This link doesn't resolve, and shouldn't it say Vec<Component> here instead?

Comment on lines +110 to +111
/// If there is an action row available the button will be added to it
/// else a new action row will be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might end up being confusing for users. We should better document how this builder should be used, and whether or not it's better to use just an ActionRow builder.

If a user is responding to an interaction with a Modal, they should use ComponentsBuilder, and add multiple TextInput components. These don't belong in an action row.

If a user is responding to an interaction with a message that includes components, and they want one action row with multiple buttons or they want one action row with a select menu, they should use the ActionRowBuilder.

If a user is responding to an interaction with a message that includes multiple action rows each with buttons, they should use ComponentsBuilder. Additionally, I think the automatic fallover behavior that is currently implemented would be confusing and could cause invalid behavior down the line, and we should prefer explicit behavior. Users should be required to add buttons or select menus directly to the place they expect them to end up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should the first action row be created by the builder or by the user too?

Copy link
Contributor

Choose a reason for hiding this comment

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

The user.

@@ -0,0 +1,15 @@
//! Component builder thing
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a temporary doc

Comment on lines +693 to +694
/// Returns an error of type [`SelectOptionValueLength`] error type if
/// a provided select option value is too long.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Returns an error of type [`SelectOptionValueLength`] error type if
/// a provided select option value is too long.
/// Returns an error of type [`SelectOptionValueLength`] if a provided select
/// option value is too long.

@7596ff
Copy link
Contributor

7596ff commented Jun 7, 2022

Any progress on this?

@itohatweb
Copy link
Member Author

Yes, I just need to update the docs. I've had much to do the last few days I will push an update soon.

@zeylahellyer zeylahellyer added the w-do-not-merge PR is blocked or deferred label Feb 4, 2023
@zeylahellyer zeylahellyer marked this pull request as draft February 4, 2023 17:31
@zeylahellyer
Copy link
Member

Triage: deferring updating + merging to likely 0.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-util Affects the util crate c-validate Affects the validate crate t-feature Addition of a new feature w-do-not-merge PR is blocked or deferred
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants