-
Notifications
You must be signed in to change notification settings - Fork 38
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
Passing component parameters to subcomponents and content #51
Comments
Hi @vsajip,
|
Thanks, for the suggestion, Ben. Though that might work in this simple case, I'm not sure it's more generally applicable. According to the docs,
I think that in your example, setting the There's another issue - #44 - where I agree that one shouldn't pass the context automatically into every component, as that would make things hard to reason about and lead to unwanted coupling. However, In my case, I want values that are known about at the component level ( I created #52 to fix this issue and it's working for me, so far at least 😄 |
Yeah that's definitely a good point. I wanted to propose a potential workaround as a temporary solution in case this issue takes time to resolve, and you'd prefer not to fork the package. For example your suggested fix might be better off looking more like this instead:
Probably more of a question for the package maintainers however. |
Well, you have to fork it to propose a PR 😄 and that's all I intended.
Is it? I see it as somewhat analogous to functions in that both are used to break bigger stuff down into smaller, more manageable chunks. Sort of like this: def component(param1, param2):
# You should be able to use param1 and param2 values here, which is the "children" bit
param3 = ... # analogous to a var directive
# and pass to subcomponent, which can't automatically see param1 and param2
# but can see param value
subcomponent(param=param3) Edit: removed mention of globals |
Is |
Sorry, for clarity I meant if you wanted a solution which didn't involve having to pin a forked package, and yes props was just an idea I was trying to express. I did think this could be more complicated depending on what Slipper's expected behaviour is in terms of whether this is a bug or an issue needing to be resolved pending decisions around design patterns, I figured it would be the latter as it's still a still a pre-1.0 package, so my intetnions was to try and help out in the meantime. Thinking about it more I guess my only main concern is around variable scope effecting code readability and maintainbility, if developers were to follow this pattern then it could be harder for developers to understand the data flow within components (e.g. not having a clear indication of their origin), the props example was suppose to highlight how you could support your change with less ambiguitly around variable scope. I thought you might be introducing issues with variable shadowing, however, it seems your changes likely follow Django templates expected behaviour anyway, for example what happens if a another
Which value would a developer expect I also thought there could be potential issues around altering parameters in the component itself, for example what if the component code for the card example above looked like this:
and I define the component this way with a
The behaviour here would be somewhat confusing, should category be "Tech", or should it render as "Fashion", but then again this might also follow what Django template tags expect, perhaps your PR will need another test to confirm the expected behaviour works. |
Sure, I agree that providing clarity for developers about how the system behaves is key. I made the change locally so I could progress the project where I'm using Slippers - I'm new to Slippers but planning to use it quite heavily to simplify some quite gnarly templates, and I'll be looking out for surprises. In terms of adding tests etc., I thought to wait and see what @mixxorz might have to say about all this. |
With the changes in my PR, I can illustrate the various possibilities. Assume the component <div>
{% var foo=foo|default:"Var foo" %}
{% var bar=bar|default:"Var bar" %}
In component, before children:
<pre class="mb-0">
Foo: {{ foo }}
Bar: {{ bar }}
</pre>
Children:
<div class="pl-4">
{{ children }}
</div>
</div> and let the instantiation be <pre>
Foo: {{ foo }}
Bar: {{ bar }}
</pre>
<hr>
{% #tester foo="Parameter foo" %}
<pre>
Foo: {{ foo }}
Bar: {{ bar }}
</pre>
{% /tester %}
<hr>
{% #tester bar="Parameter bar" %}
<pre>
Foo: {{ foo }}
Bar: {{ bar }}
</pre>
{% /tester %}
<hr> And let only
I expect it should be possible to make |
I've been thinking about this. I am not yet sure what I think about this and expect I'll need some time to form a strong opinion. One approach I've explored is not doing this by default and using a special keyword to enable this functionality.
The keyword is |
The reason for me suggesting that it be the default is that there is no other way of getting variable/parametric information into component content which is not in subcomponents (whereas one could choose to pass parameters as e.g. |
We might disagree here, but for me that is a good thing. I do not want component children to have a dependency on the component's context because you can end up breaking the rendering of children when you introduce changes at the component-level. Components will no longer be self-contained. Changing the component code could now have side-effects elsewhere in your code. With the current implementation of Slippers, you can be sure that whatever happens to the component, your children will be rendered just as you passed them and you do not need to worry about side-effects. I am leaning towards keeping the children decoupled from the component, at least by default. |
That is always true, is it not? For example, a breaking change could still be introduced into a component regardless of the resolution of this issue. "Breaking" in the sense that behaviour can change in an unexpected/surprising way after a change to a component. |
Also, @mixxorz, what alternative method do you propose to address my use case in the initial post in this issue? |
Consider this component (named
tester
):Instantiated like this:
Results in
Clearly the intent is for the
h1
to haveid="bar"
, but it's not happening, seemingly because the component context isn't available when rendering the children. Is there some way to pass this context that I'm missing? To be clear, I mean just the context passed to the component -id
andlabelid
.The text was updated successfully, but these errors were encountered: