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

Add the Conditionable trait to the FormBuilder #152

Closed

Conversation

ash-jc-allen
Copy link

Hey! This PR proposes adding the Illuminate\Support\Traits\Conditionable trait to the Laravel\Prompts\FormBuilder class.

I love using when in my Laravel apps so I can chain my method calls together. So I reached out today to use it when building a form in Prompts and noticed it didn't exist.

Here's an example use case of a form using a standard if statement to add a new user to the app:

// Imagine this is coming from config or something like that...
$smsEnabled = true;

$form = form()
    ->text(
        label: 'Email:',
        required: true,
        validate: ['email'],
        name: 'email'
    );

if ($smsEnabled) {
    $form->text(label: 'Phone number:', name: 'phone_number');
    $form->confirm(label: 'Send Welcome SMS?', name: 'send_sms');
}

$formData = $form->submit();

Using when it could be rewritten like so:

// Imagine this is coming from config or something like that...
$smsEnabled = true;

$formData = form()
    ->text(
        label: 'Email:',
        required: true,
        validate: ['email'],
        name: 'email'
    )
    ->when(
        $smsEnabled,
        function (FormBuilder $form): void {
            $form->text(label: 'Phone number:', name: 'phone_number');
            $form->confirm(label: 'Send Welcome SMS?', name: 'send_sms');
        },
    )
    ->submit();

I did notice that Prompts isn't specific to Laravel and can be used in other PHP apps, so I don't know if keeping the dependencies lightweight will play a part in the decision to merge this. But from looking at the Composer files, I think Prompts includes illuminate/collections which in turn requires illuminate/conditionable. So if I'm right, it means we already have access to this trait without needing to add any new dependencies.

If this is something you think might be helpful to other devs, please give me a shout if you might want anything changed on this PR. If not, that's totally cool, at least I gave it a shot! 😄

@driesvints
Copy link
Member

driesvints commented May 31, 2024

Sorry @ash-jc-allen but I want to advocate against this change as we're actually looking at removing the illuminate/support dependency entirely in the near future. It's now a circular dependency with framework core and we want to get rit of this. Adding functionality now which we'll have to remove later on doesn't help 😞

@ash-jc-allen
Copy link
Author

Hey @driesvints! That's totally cool, I had a feeling the dependencies might have been a bit of a concern 🙂

@driesvints
Copy link
Member

Cool. Gonna close this then.

@driesvints driesvints closed this May 31, 2024
@ash-jc-allen ash-jc-allen deleted the feature/conditionable branch May 31, 2024 10:52
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.

2 participants