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

New when() helper. #52665

Merged
merged 15 commits into from
Sep 11, 2024
Merged

New when() helper. #52665

merged 15 commits into from
Sep 11, 2024

Conversation

danmatthews
Copy link
Contributor

@danmatthews danmatthews commented Sep 5, 2024

Based on a little twitter discussion with Josh Cirre and @joshhanley over on twitter - it was his idea really!

Adds a new when() helper method to make this work, but this could either be exposed as a helper, not exposed as a helper, renamed to something else like printWhen or echoWhen if it keeps things less cluttered.

Use Case Example

Useful for attributes and more, the example we explored in the thread was for conditionally printing a livewire binding:

<div @when($condition, 'wire:poll.5s="myMethodName"', false)>

But could also be useful for conditionally printing things in general:

<input name="phone" @when($isRequired, 'required') />

@danmatthews danmatthews changed the title feat(blade): New @when directive. New blade @when directive. Sep 5, 2024
@rodrigopedra
Copy link
Contributor

Shouldn't escaping be on by default?

Copy link
Contributor

@joshhanley joshhanley left a comment

Choose a reason for hiding this comment

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

Love it! Nice work @danmatthews.

Have made some suggestions for a few small fixes 🙂

src/Illuminate/Foundation/helpers.php Outdated Show resolved Hide resolved
src/Illuminate/Foundation/helpers.php Outdated Show resolved Hide resolved
src/Illuminate/View/Compilers/Concerns/CompilesWhens.php Outdated Show resolved Hide resolved
src/Illuminate/View/Compilers/Concerns/CompilesWhens.php Outdated Show resolved Hide resolved
@PhiloNL
Copy link
Contributor

PhiloNL commented Sep 6, 2024

Love it! +1 This looks much cleaner compared to having all these inline @if and @endif directives. Especially when using Livewire, it makes things less bloated when dealing with larger components.

@devajmeireles
Copy link
Contributor

devajmeireles commented Sep 6, 2024

I see that this would bring benefits, but I believe it would be interesting to consider the following:

  1. @when and also @unless
  2. The logic of $output was similar to the ComponentAttributeBag:
<div @when($true, ['wire:poll.5s' => 'myMethodName'])>

<div @unless($false, ['wire:poll.5s'])>

Also, the example:

<input name="phone" @when($isRequired, 'required') />

... would probably be discarded since the framework already has @required

@danmatthews
Copy link
Contributor Author

danmatthews commented Sep 6, 2024

I believe the framework already has unless, but it's a block scoped statement:

@unless($something)
Hello
@endunless

And i'm not sure you can have both a block scoped statement AND a single tag directive with the same name.

Good point with required though, always forget about @required

@taylorotwell
Copy link
Member

I wonder if there are any gotchas to this being echoed a bit differently compared to @if @endif. Especially around escaping, etc.

@joshhanley
Copy link
Contributor

@taylorotwell yeah maybe people would expect @when(true) echo something @endwhen to work when it wouldn't.

I wouldn't be against just having the when() helper instead of the blade directive, so we can do

<div {{ when($someCondition, 'wire:poll') }}>

Or even possibly change @if to accept extra parameters, like @when in this PR instead of @when?

@danmatthews
Copy link
Contributor Author

danmatthews commented Sep 9, 2024

@taylorotwell @joshhanley what about setting a new blade 'paradigm' with @printWhen(), then we could have @printUnless also? That removes the gotchas of these being associated with conditionals and makes it more explicit what they're doing?

Edit: Paradigm is the wrong word, but including "print" in the directive name would mean that we might have to enforce this going forward on any directives that do 'echo' or 'print' things, and if that's at odds with current ones that do print things without this prefix.

Edit 2: For simplicity, i'm actually down with @joshhanley 's suggestion of the helper, then the print logic using {{ }} or {!! !!} would be exactly what you'd expect.

@taylorotwell
Copy link
Member

@joshhanley I would maybe start with just the when function, and I think you could remove the escape stuff from it entirely since that wouldn't be needed anymore if you're just using {{ }} or {!! !!}. Please mark as ready for review when the requested changes have been made.

@taylorotwell taylorotwell marked this pull request as draft September 9, 2024 14:27
@danmatthews danmatthews changed the title New blade @when directive. New when() helper. Sep 9, 2024
@danmatthews
Copy link
Contributor Author

danmatthews commented Sep 9, 2024

  • Removed the @when directive compilation code.
  • Removed tests for @when blade directive.
  • Added tests for the when() helper.
  • Removed any escaping logic.
  • Renamed the PR.

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Sep 9, 2024

How about adding a default value, instead of defaulting to null? That would align better with the method on Illuminate\Support\Traits\Conditionable@when()

As this is now a helper, I guess the return type-hint could be dropped, so it won't cast to a string when used in a context other than a blade template.

Also, I would drop the bool type-hint on the $condition and rely on truthy/falsy values.

function when($condition, $value, $default = null)
{
    if ($condition) {
        return value($value, $condition);
    }

    return value($default, $condition);
}

This way, one could do something like this:

{{ when($user, fn ($user) => $user->preference('theme'), 'light') }}

And simple usage would still work:

{{ when($user->meetsSomeCriteria(), 'wire:poll.5s="myMethodName"') }}

@danmatthews
Copy link
Contributor Author

danmatthews commented Sep 9, 2024

@rodrigopedra I like it, i've got a refactor that matches this on the go, but would love to hear from @joshhanley and/or @taylorotwell what they think about

  • providing a default option, and if not, what to return if the condition fails, null or just void / noreturn - for example i believe something like Arr::get() with no default would return null.
  • Passing the condition object in as an argument to the closure if you use a closure for value .

@timacdonald
Copy link
Member

timacdonald commented Sep 10, 2024

It is silly how annoyed I get whenever I have to do the {{ $condition ? 'thing' : '' }} song and dance.

Dig this.

@joshhanley
Copy link
Contributor

@danmatthews PR's looking good! I'm with @timacdonald it really annoys me having to do that too haha 😂

Re the default, while I see it's benefit, I also think that if you need a default, then just revert back to using the ternary operator {{ $condition ? 'thing' : 'default' }}. But not against it either.

@rodrigopedra is right about the return type, I don't think it should be restricted to just a string. As I will use when() in php as well as in blade and it might return arrays or objects, etc. So I would remove the return type altogether and update the docblock to be mixed.

So keen to be able to use this 😁

@danmatthews
Copy link
Contributor Author

Cool, i've removed the default, but kept the value() helper so you can pass a closure. I've omitted passing the $condition value into the value arguments, as most of the time it would just be true|false and likely you've got access to anything you need in scope anyway during the function call or render in blade:

{{ when($user, fn() => $user->name) }}

Currently it returns null to avoid the pitfalls of a no-return function when assigning to a variable etc.

@danmatthews danmatthews marked this pull request as ready for review September 11, 2024 08:28
@taylorotwell taylorotwell merged commit d9c322d into laravel:11.x Sep 11, 2024
30 of 31 checks passed
@taylorotwell
Copy link
Member

Thanks!

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.

7 participants