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

[8.x] @aware blade directive #39100

Merged
merged 12 commits into from
Oct 6, 2021
Merged

[8.x] @aware blade directive #39100

merged 12 commits into from
Oct 6, 2021

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Oct 5, 2021

This is a re-submit of #39080 that makes the data from any components that are actively being rendered as well as parent components that haven't yet been rendered available to child components via the @consume directive:

{{-- menu.blade.php --}}
@props(['color'])
<ul>
  <x-menu-item color="orange">A</x-menu-item>
  <x-menu-item>B</x-menu-item>
  {{ $slot }}
</ul>

{{-- menu-item.blade.php --}}
@consume(['color' => 'red'])
<li>slot={{ $slot }}, color={{ $color }}</li>

{{-- Without setting color attribute, colors will be: orange, red, blue, red --}}
<x-menu>
  <x-menu-item color="blue">C</x-menu-item>
  <x-menu-item>D</x-menu-item>
</x-menu>

{{-- With setting color attribute, colors will be: orange, pink, blue, pink --}}
<x-menu color="pink">
  <x-menu-item color="blue">C</x-menu-item>
  <x-menu-item>D</x-menu-item>
</x-menu>

@taylorotwell
Copy link
Member

Heh, didn't know you were working on this again and actually just submitted a PR with another approach.

Let me know what you think: https://github.com/laravel/framework/pull/39102/files

@inxilpro
Copy link
Contributor Author

inxilpro commented Oct 5, 2021

Hm. What do you expect the output of my tests to be? With your implementation, I'm expecting:

<ul>
<li>slot=Inline child 1, color=orange</li>
<li>slot=Inline child 2, color=pink</li>
<li>slot=Default item 1, color=blue</li>
<li>slot=Default item 2, color=pink</li>
</ul>

But actually getting:

<ul>
<li>slot=Inline child 1, color=orange</li>
<li>slot=Inline child 2, color=orange</li>
<li>slot=Default item 1, color=blue</li>
<li>slot=Default item 2, color=blue</li>
</ul>

It feels like you should be able to explicitly override consumed data. A use case may be:

<x-menu style="muted">
  <x-menu.item>About</x-menu.item>
  <x-menu.item>Contact</x-menu.item>
  <x-menu.item style="danger">Log Out</x-menu.item>
</x-menu>

@inxilpro inxilpro mentioned this pull request Oct 5, 2021
@taylorotwell
Copy link
Member

taylorotwell commented Oct 5, 2021

@inxilpro can you pull in my recent flushComponent stuff on 8.x into this branch?

@inxilpro
Copy link
Contributor Author

inxilpro commented Oct 5, 2021

@taylorotwell I just updated the tests. I tried to merge yours and mine together so that they covered all the possible cases. Let me know if you have an issue with that. I'll pull in the flushComponent bit shortly.

@inxilpro
Copy link
Contributor Author

inxilpro commented Oct 6, 2021

There are two things that I thought of that might need consideration before merge.

The first is my comment on the docs PR — defaults set in the @props directive are not available to the @aware directive. They need to be set in both places.

The second is whether we want to add an $aware property to Illuminate\View\Component that gets merged along with public properties and methods, so that you can do:

class MenuItem extends Component
{
  public $aware = ['color' => 'gray'];

  // ...
}

@inxilpro
Copy link
Contributor Author

inxilpro commented Oct 6, 2021

(I can't really think of any reasonable way to get around the @props issue. The parent's view can't be evaluated until its children are.)

@taylorotwell
Copy link
Member

@inxilpro I don't think those are major issues. I personally would not add an $aware property at the moment. The @props default issue we will just have to document.

@taylorotwell taylorotwell merged commit ddb5fef into laravel:8.x Oct 6, 2021
chu121su12 pushed a commit to chu121su12/framework that referenced this pull request Oct 8, 2021
* initial pass at consume directive

* rename method

* Track render data for consume

* Add more variety to tests

* formatting

* Update tests

* Handle flushComponents

* rename to aware

* formatting

* formatting

Co-authored-by: Taylor Otwell <taylorotwell@gmail.com>
chu121su12 pushed a commit to chu121su12/framework that referenced this pull request Oct 8, 2021
* initial pass at consume directive

* rename method

* Track render data for consume

* Add more variety to tests

* formatting

* Update tests

* Handle flushComponents

* rename to aware

* formatting

* formatting

Co-authored-by: Taylor Otwell <taylorotwell@gmail.com>
@driftingly
Copy link

@inxilpro are you able to update the name/description of this PR to match the @consume => @aware rename?

@inxilpro inxilpro changed the title [8.x] Consume Blade Directive [8.x] @aware blade directive Oct 8, 2021
victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
* initial pass at consume directive

* rename method

* Track render data for consume

* Add more variety to tests

* formatting

* Update tests

* Handle flushComponents

* rename to aware

* formatting

* formatting

Co-authored-by: Taylor Otwell <taylorotwell@gmail.com>
@thewebartisan7
Copy link
Contributor

thewebartisan7 commented Feb 10, 2022

With component class you still use @aware directive in view right?

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