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

Adds support for conditional steps #171

Merged
merged 5 commits into from
Oct 9, 2024

Conversation

jochem-blok
Copy link
Contributor

@jochem-blok jochem-blok commented Sep 25, 2024

In pull request #118 revertable forms is introduced. I found no proper way to make steps conditional to the answer of the previous steps. The $step closure seems to be an option. But then reverting does not work.

See the example below:

use function Laravel\Prompts\form;
use function Laravel\Prompts\text;

$responses = form()
    ->text('What is your name?')
    ->select('What is your language?', ['PHP', 'JS'], name: 'language')
    ->add(function ($responses) {
        if ($responses['language'] == 'PHP') {
            return text("Which version?");
        }
    },
        name: 'phpversion',
    )
    ->add(function ($responses) {
        if ($responses['language'] == 'JS') {
            return text("Which os?");
        }
    },
        name: 'os'
    )->confirm('Are you sure?')
    ->submit();

afbeelding

The option ignoreWhenReverting seems to be an option, but it that simply skips the step. So conditional forms are hard.

In the example below I rewrote the above example with the suggested change. And now everything seems to work fine.

use function Laravel\Prompts\form;
use function Laravel\Prompts\text;

$responses = form()
    ->text('What is your name?')
    ->select('What is your language?', ['PHP', 'JS'], name: 'language')
    ->add(function () {
            return text("Which version?");
        },
        name: 'phpversion',
        condition: function ($responses) {
            return $responses['language'] == 'PHP';
        }
    )
    ->add(function () {
            return text("Which os?");
        },
        name: 'os',
        condition: function ($responses) {
            return $responses['language'] == 'JS';
        }
    )->confirm('Are you sure?')
    ->submit();

afbeelding

@jochem-blok jochem-blok changed the title add support for conditional forms add support for conditional steps Sep 25, 2024
@taylorotwell
Copy link
Member

Drafting pending review from @jessarcher

@taylorotwell taylorotwell marked this pull request as draft September 25, 2024 15:16
@jessarcher jessarcher marked this pull request as ready for review September 26, 2024 01:44
@crynobone crynobone changed the title add support for conditional steps Adds support for conditional steps Sep 26, 2024
@taylorotwell
Copy link
Member

Should this instead be something like addIf where the condition can be the first argument?

@taylorotwell taylorotwell marked this pull request as draft September 27, 2024 15:08
@jochem-blok
Copy link
Contributor Author

Is this something I should answer? This is my first PR :)

I am happy to implement the suggested change.

@devajmeireles
Copy link
Contributor

Is this something I should answer? This is my first PR :)

I am happy to implement the suggested change.

He's asking a general question, to all PR participants (including you), and also to the main maintainer, @jessarcher

@jessarcher
Copy link
Member

Having the condition as the first argument reads nicely.

I think the ultimate API would be a when method that gets the form instance:

$responses = form()
    ->text('First question...')
    ->when($condition, fn ($form) => $form
        ->text('Conditional question')
        ->text('Another conditional question')
    );

But I had a quick play around and it's not straight-forward to implement with the revert behavior.

@jochem-blok
Copy link
Contributor Author

jochem-blok commented Sep 30, 2024

Following your reasoning Taylor why isn't addScrollable (because scroll is also an option) added? Having each optional parameter in a function would create a cartesian product.

And with the option to use named arguments I don't see the need for moving the condition to the first argument (and remove the optional behaviour).

How do we advance in this? Jess is trying to implement the same effect with another solution.

@jessarcher
Copy link
Member

Following your reasoning Taylor why isn't addScrollable (because scroll is also an option) added? Having each optional parameter in a function would create a cartesian product.

I'd say it's because the conditional aspect, when present, is important to read before the specific configuration of the step. It follows the way a typical if statement reads (if, then...) and matches things like the abort_if function in the framework.

How do we advance in this? Jess is trying to implement the same effect with another solution.

I think Taylor would be happy to merge if you added a new addIf($condition, ...) method, instead of changing the add method.

@jochem-blok jochem-blok marked this pull request as ready for review October 4, 2024 11:38
@jochem-blok
Copy link
Contributor Author

I implemented the new method and changed the test cases. Please review :)

@jochem-blok jochem-blok requested a review from jessarcher October 4, 2024 11:40
@taylorotwell taylorotwell marked this pull request as draft October 4, 2024 14:05
src/FormBuilder.php Outdated Show resolved Hide resolved
@jessarcher jessarcher marked this pull request as ready for review October 9, 2024 12:19
@taylorotwell taylorotwell merged commit 7f2060e into laravel:main Oct 9, 2024
7 checks passed
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.

4 participants