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] Blade component slot attributes #38372

Merged
merged 12 commits into from
Aug 18, 2021
Merged

[8.x] Blade component slot attributes #38372

merged 12 commits into from
Aug 18, 2021

Conversation

danharrin
Copy link
Contributor

This PR further enhances the power of Blade components by allowing slots to have their own attributes. 🚀

Take this card component example:

@props([
    'heading',
    'footer',
])

<div {{ $attributes->class(['border']) }}>
    <h1 {{ $heading->attributes->class(['text-lg']) }}>
        {{ $heading }}
    </h1>

    {{ $slot }}

    <footer {{ $footer->attributes->class(['text-gray-700']) }}>
        {{ $footer }}
    </footer>
</div>
<x-card class="shadow-sm">
    <x-slot name="heading" class="font-bold">
        Heading
    </x-slot>

    Content

    <x-slot name="footer" class="text-sm">
        Footer
    </x-slot>
</x-card>

In this example, I am able to fully customise multiple areas of the Blade component, not just its outer container. By passing the class attribute to both slots, I can make the heading bold and the footer smaller without having to create another view just for this use case.

Let me know if you have any questions about this implementation, or you can see any areas that could be improved!

@dennisprudlo
Copy link
Contributor

That would be awesome if merged. Currently I use some custom props like inner-class or <slotname>-class. This would cleanup the components a lot!

@taylorotwell
Copy link
Member

taylorotwell commented Aug 13, 2021

While I appreciate this feature, I want to really make sure it's needed. In both of your examples could you not have just done this type of thing:

    <x-slot name="heading">
        <span class="font-bold">Heading</span>
    </x-slot>

Does Vue support this?

@stancl
Copy link
Contributor

stancl commented Aug 13, 2021

@taylorotwell Remember when we talked about scoped slots for Blade? In the implementation I used (https://twitter.com/samuelstancl/status/1390655396927447043), I had no way to access other attributes on the <x-slot> call. So I had to use <x-slot name="scoped:foo">

Dan's feature would let me use:

<x-slot name="foo" scoped>

</x-slot>

I'm actually happy to contribute the scoped slots feature if Dan's PR is merged. The code is super simple and the only ugly bits currently are the ones that check the scoped: prefix in the name — which wouldn't be a thing after this change.

@danharrin
Copy link
Contributor Author

Thanks for the consideration! This is definitely needed.

The example I gave was purely of the implementation really, I agree that it's not a good example to demonstrate its power.

Where this really comes in useful is outside of simple class territory. For example - if the wrapper around the slot is not a simple <div> and has more specialised attributes, you may want to pass attributes to that element - which cannot be done by creating a child element.

<textarea {{ $textarea->attributes }}>
    {{ $textarea }}
</textarea>

And - Vue does not support this yet, but I saw someone mention earlier that they'd love it if it was 😅

@binaryweavers
Copy link

As per need it's good to have when you extract the components in a package with most of the functionality implemented in component and you might want to avoid extra markup.
Another example can be a dropdown component having activator which can have own styling and html tag etc 🤔
We are currently using this in our projects where we have extracted UI components and just pull them in

@php

$activatorTag = $activtor->attributes->get('tag', 'div');

@endphp
<{{ $actrivatorTag }}
      {{ $activator->attributes->except(['tag'])->class(['transition-all']) }}
      x-ref="activator"
      x-on:click="openOnClick && !isActive ? show() : hide()"
      x-on:focus.debounce.150ms="openOnFocus && show()">

        {{ $activator }}

</{{ $actrivatorTag }}>
<x-dropdown>

     <x-slot name="activator" class="..."  tag="button" disabled>

            Settings

     <x-slot>

     ...
    
</x-dropdown>

@binaryweavers
Copy link

@danharrin I see that implementation is almost same as we are using. We faced an edge case when using this approach in DynamicComponent .

protected function compileSlots(array $slots)

    protected function compileSlots(array $slots)
    {
        return collect($slots)->map(function ($slot, $name) {
            return $name === '__default' ? null : '<x-slot name="'.$name.'">{{ $'.$name.' }}</x-slot>';
        })->filter()->implode(PHP_EOL);
    }

I solved this overriding dynamic component like so
https://github.com/binaryweavers/laravel-blade-slots-plus/blob/25c33259cf5f60b2b18c70e222e50397fdeac0c6/src/DynamicComponent.php#L35

    protected function compileSlots(array $slots)
    {
        return collect($slots)->map(function ($slot, $name) {
            $slot->attributes = ($slot->attributes instanceof ComponentAttributeBag) ? $slot->attributes : new ComponentAttributeBag();
            $slot->attributes->setAttributes($slot->attributes->getAttributes());
            return $name === '__default' ? null : '<x-slot name="' . $name . '" ' . $slot->attributes->__toString() . '>{{ $' . $name . ' }}</x-slot>';
        })->filter()->implode(PHP_EOL);
    }

@taylorotwell
Copy link
Member

@danharrin any thoughts on the dynamic component note above?

@danharrin
Copy link
Contributor Author

@taylorotwell I'll add a test for dynamic components to check it's an actual issue, because looking at @binaryweavers code, I can't see why that would do anything but replace the already existing slot attribute bag 😅 it should be created when the slot is constructed, so should always be present.

@binaryweavers
Copy link

@taylorotwell I'll add a test for dynamic components to check it's an actual issue, because looking at @binaryweavers code, I can't see why that would do anything but replace the already existing slot attribute bag 😅 it should be created when the slot is constructed, so should always be present.

Haha TBH, I am not sure why I did that, I was lacking time and was facing an issue as well (you see I don't have tests as well 🤦‍♂️).
I'll try to reproduce why I did that and update 👍🏼
As I am overriding native stuff a lot but something was causing issue that time which got fixed this way. I was on code & fix strategy at that time 😂

@danharrin
Copy link
Contributor Author

Looks like this works fine if you cast the attributes to a string and pass them through normally 👍 No need to reset the attribute bag.

@binaryweavers
Copy link

Okay, the actual problem with dynamic components is rooted here, so I don't know how tf @binaryweavers' code actually worked ahah 😅 I'll think about the best way to fix this, but I don't think it's gonna be too easy :(

Hahah don't worry I got you covered and backtraced that confusing part of mine code 😂 I see that you missed that too. Here is the breakdown first.
Just like you also forgot to update this line

$defaultSlot = new HtmlString(trim(ob_get_clean()));
and $slot->attributes were not working as it gets collected in componentData method.
then I blindly patched those confusing lines in DynamicComponent 🤦‍♂️
Then later on I updated here . So Those two line are just doing nothing there now.

@danharrin
Copy link
Contributor Author

@binaryweavers @taylorotwell I think I've fixed it, but have a check yourselves if you wish!

@binaryweavers
Copy link

Looks like this works fine if you cast the attributes to a string and pass them through normally 👍 No need to reset the attribute bag.

Yes they are unnecessary after making default $slot an instance of ComponentSlot in componentData method

@taylorotwell
Copy link
Member

Note that XSS is possible due to bound attribute escaping being disabled:

image

image

image

@driesvints
Copy link
Member

@danharrin tests are failing

@danharrin
Copy link
Contributor Author

@taylorotwell I tested your example and, as far as I can see, escaping bound attributes has no effect on slot attribute output or component attribute output. Am I missing something?

Screenshot 2021-08-16 at 16 39 13

Screenshot 2021-08-16 at 16 39 31

Screenshot 2021-08-16 at 16 39 45

@taylorotwell
Copy link
Member

taylorotwell commented Aug 16, 2021

Clear your compiled views. It does escape for me when I switch to escapeBound = true on slot data.

@danharrin
Copy link
Contributor Author

danharrin commented Aug 16, 2021

Clear your compiled views. It does escape for me when I switch to escapeBound = true on slot data.

Yeah I had cleared them and the issue was there to start with. It's all good as long as you're happy with it working as you expect :)

@taylorotwell
Copy link
Member

taylorotwell commented Aug 18, 2021

Hmm - there is still the XSS vulnerability in the PR? Any user provided data that is bound such as alt text, etc. will be vulnerable to XSS.

@taylorotwell
Copy link
Member

Ah, nevermind, I see you started escaping them now.

@taylorotwell taylorotwell merged commit b75a01f into laravel:8.x Aug 18, 2021
@danharrin danharrin deleted the feature/blade-component-slot-attributes branch August 19, 2021 13:54
victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
* Update slot pattern

* Add attributes params to existing tests

* Create ComponentSlot class

* Pass attributes from slot tag to Blade directive

* Compile slot attributes into slot object

* Add compilation tests for attribute support

* Remove unused exception

* Reorder arguments

* Fix dynamic components with slot attributes

* Escape bound attributes for slots

* Update BladeComponentTagCompilerTest.php

* formattinG

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

thewebartisan7 commented Jan 23, 2022

Nice feature that I missed time ago. Thanks.

Seem not possible to pass to a component the slot attributes, like:

<div {{ $attributes) }}>

   <x-mycomponent {{ $myslot->attributes->class('my-class') }}>{{ $myslot }}</x-mycomponent>

    {{ $slot }}

</div>

@thewebartisan7
Copy link
Contributor

This doesn't work:

<x-my-component {{ $myslot->attributes }}>{{ $myslot }}</x-my-component>

While this works:

<x-my-component :attributes="$myslot->attributes">{{ $myslot }}</x-my-component>

But with main $attributes this works:

<x-my-component {{ $attributes }}>{{ $slot }}</x-my-component>

@danharrin is this expected behaviour?

The error message is: "syntax error, unexpected token "else", expecting end of file "

@dennisprudlo
Copy link
Contributor

dennisprudlo commented Feb 10, 2022

@thewebartisan7 This components rendering with arbitrary blade-injected content is not possible at the moment. You can't use {{ $something }} in the component tag as well as you can't use blade directives such as this:

<x-my-component @if ($condition) class="text-red-500" @endif>
    {{ $slot }}}}
</x-my-component>

Your second example works because you bind $myslot->attributes directly to a prop and the third example works because it is explicitly allowed (because passing $attributes down that way is a very common and ultimately needed use-case):

https://github.com/laravel/framework/pull/38372/files#diff-bddd3c3babe9b0302fb60bf7836b9177618530224733a4f20ef5286313eec6dcR408-R410

@thewebartisan7
Copy link
Contributor

That make sense. Thanks.

@yaddly
Copy link

yaddly commented Jan 13, 2023

This PR further enhances the power of Blade components by allowing slots to have their own attributes. 🚀

Take this card component example:

@props([
    'heading',
    'footer',
])

<div {{ $attributes->class(['border']) }}>
    <h1 {{ $heading->attributes->class(['text-lg']) }}>
        {{ $heading }}
    </h1>

    {{ $slot }}

    <footer {{ $footer->attributes->class(['text-gray-700']) }}>
        {{ $footer }}
    </footer>
</div>
<x-card class="shadow-sm">
    <x-slot name="heading" class="font-bold">
        Heading
    </x-slot>

    Content

    <x-slot name="footer" class="text-sm">
        Footer
    </x-slot>
</x-card>

In this example, I am able to fully customise multiple areas of the Blade component, not just its outer container. By passing the class attribute to both slots, I can make the heading bold and the footer smaller without having to create another view just for this use case.

Let me know if you have any questions about this implementation, or you can see any areas that could be improved!

Good day and thank you for this invaluable and educative post. May I ask if it is possible to check if a named slot has data/content before rendering its content? In other words, how do I make named slots optional? An if statement using the named slot throws an error, your guidance would be highly appreciated.

@if(!$footer->isEmpty())//This line throws Undefined variable $footer
{
    {{$footer}}
}
@endif

However, when you check for emptiness on the main slot, it works.

@if(!$slot->isEmpty())
{
     {{$slot}}
}
@endif

Without the ability to make named slots optional, one is now compelled to litter code with empty named slots just to avoid errors

<x-slot name="footer"></x-slot>

@danharrin
Copy link
Contributor Author

If your slot is nullable, you should delare it in @props(['footer' => null])

Then you can just use @if ($footer?->isNotEmpty())

@yaddly
Copy link

yaddly commented Jan 13, 2023

If your slot is nullable, you should delare it in @props(['footer' => null])

Then you can just use @if ($footer?->isNotEmpty())

Thank you very much for guiding me into resolving the error I had.

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.

8 participants