symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
828 stars 299 forks source link

[Twig Components] - scoped props ? #1970

Closed barberob closed 2 months ago

barberob commented 3 months ago

(i'm using only anonymous components)

Hey, the current behavior is that inner components can acces their parents props. This is useful but this creates some edge cases that can be annoying. For exemple, i'm creating a sort of UI library and some nested components can share attributes with same name like in my case : icon Here my badge can have an icon sometime but i dont want one in this case, so i have to set it to null

//--------- in both my components

{% props icon = null, ..... %}

{# ... some code  ... #}

//------- in my template

<twig:Nav:Link icon="folder-plus">

    {# ... what i'd like to do  ... #}
    <twig:Badge>.......</twig:Badge>

    {# ... what I have to do ( i guess ? )  ... #}
    <twig:Badge :icon="null">.......</twig:Badge>

</twig:Nav:Link>

I would like to avoid giving different name to same prop on each of my components ( badgeIcon, buttonIcon, ..... ) or that weird null thing Is there a way to make some specific props "private" or "scoped", as they would be defined in the components, and only accessible in this component ant not it's children. In my ideal world, i think it would be great to have something like :

{# could be a function or a fiter #}
{% props scoped(icon = null), ..... %}

{# or a second twig tag specific to this behavior #}  
{% scopedProps icon = null, ... %}

Does it make sense ?

smnandre commented 3 months ago

Hmm i cannot reproduce your problem. When i set icon on the parent, the props is not concerned in the child.

I have made two components Foo and Bar

{# templates/components/Foo.html.twig #}
{% props foo = null %}

<div style="border: 1px solid blue;padding: 1rem;">
    <h3>FOO</h3>

    foo: <pre><code>{{ foo }}</code></pre>

    {% block content %}{% endblock %}
</div>
{# templates/components/Bar.html.twig #}
{% props foo = null %}

<div style="border: 1px solid red;padding: 1rem;">
    <h3>BAR</h3>

    foo: <pre><code>{{ foo }}</code></pre>

    {% block content %}{% endblock %}
</div>

Let's call them in different scenarios

<twig:Foo foo="FOOBAR">
    <twig:Bar />
</twig:Foo>

 <twig:Foo foo="FOOBAR">
     <twig:Bar></twig:Bar>
</twig:Foo>

<twig:Foo>
    <twig:Bar foo="FOOBAR" />
</twig:Foo>

ANd results:

twig-foo-bar

smnandre commented 3 months ago

Could you create a reproducer for this bug ?

The idea is to get the smallest repository possible in which the bug can be observed... ... so we can install & test it easily and see what is not working as expected and why.

Documentation: https://symfony.com/doc/current/contributing/code/reproducer.html

Thank you!

barberob commented 3 months ago

No problem, i have a suuuuuuper weird exemple here : https://github.com/barberob/bugged_twig_components_issue_1970_ux

i have two exemples in index.html.twig :

    {# this is bugged #}
    <twig:Link icon="my-icon">
        <twig:Badge>1</twig:Badge>
    </twig:Link>

    {# this works correctly #}
    <twig:Link icon="my-icon">
        <twig:FixedBadge>1</twig:FixedBadge>
    </twig:Link>

image

where the only difference in fixed badge is that i'm not passing one more (unused) prop

smnandre commented 3 months ago

Well thank you for this, because it show either a problem in documentation, more probably a problem in props somewhere.

I made several tests to observe the results depending on how you declare your props (no default, default null, default 'my-icon") and the order of your props.

I almost told you 'it seems you need to declare your nullable prop at the end, but case E/F clearly show it's not the case.

props-defaumt

smnandre commented 3 months ago

@WebMamba & @squrious you both worked on this recently i think.. could you have a look ?

We need at least some coherence (and the example given from @barberob come straight from the documentation so we need to change/fix something there... but we need to be really precaution to not break things :|

WebMamba commented 2 months ago

Thanks for the ping! I started look at this but didn't find out why it behave like this. The parent context should never be on the chidren context.

smnandre commented 2 months ago

Looks to me related to props / default values (anonymous component)

squrious commented 2 months ago

Hi there! I think it may be related to the way props existence is checked here (cf. https://github.com/symfony/ux/pull/1820#issuecomment-2092441936).

Using isset considers the value as not set if it's null, while array_key_exists would allow overriding values from parent context when using null as a default in props definition?

WebMamba commented 2 months ago

@barberob it should be fixed now. Could you try the PR and tell me if it work for you please ? I try on your reproducer and it work well now. Thanks for your report! It looks like we missed up something because the code was there but not at the right place! 😅

barberob commented 2 months ago

@WebMamba Hey works perfecty for me :+1: Thanks for the help