-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
[TwigComponent] Add variable to refer outer scope when rendering embedded components #936
Conversation
IMO a child component should not have access to its parent. |
Well... the tricky thing is that you ARE still kind of "inside" the parent template - i.e. you're inside I'm not on one side or the other yet. |
Since #920 is merged now, I rebased this branch on the current HEAD. |
f32584d
to
1b6f89d
Compare
6f5718c
to
d8ca2ef
Compare
src/TwigComponent/doc/index.rst
Outdated
{% component Alert with { type: 'success' } %} | ||
{{ this.parent.someFunction }} {# this refers to SuccessAlert #} | ||
|
||
{{ this.parent.someProp }} {# references a "someProp" prop from SuccessAlert #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this.parent
(which requires us to use a wrapper Hierarchy
object and magic methods), what about following our outerBlocks
convention and adding an outerContext
variable?
{{ outerContext.this.someProp }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you refer this.parent.parent then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outerContext.outerContext.this.someProp
? I think this would naturally occur if we started adding the outerContext
variable, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed and using outerScope
variable instead of outerContext
as discussed via Slack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good - big set of tests 👍
src/TwigComponent/doc/index.rst
Outdated
{% set name = 'Fabien' %} | ||
{% set message = 'Hello' %} | ||
{% component Alert with { type: 'success', name: 'Bart' } %} | ||
Hello {{ name }} {# name = Bart #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be slightly more clear to just have:
Hello {{ name }} {# name = Bart #} | |
Hello {{ name }} {# Hello Bart #} |
Especially in the next example, where name = Fabien
isn't actually quite right. But showing the printed result, I think, is clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
src/TwigComponent/doc/index.rst
Outdated
{% component Alert with { type: 'success', name: 'Bart' } %} | ||
Hello {{ name }} {# name = Bart #} | ||
|
||
{{ message }} {{ outerScope.name }} {# message = 'Hello', name = Fabien #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{ message }} {{ outerScope.name }} {# message = 'Hello', name = Fabien #} | |
{{ message }} {{ outerScope.name }} {# Hello Fabien #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
src/TwigComponent/doc/index.rst
Outdated
{% endcomponent %} | ||
|
||
To keep the generic Alert component unaware of the ``someProp`` prop it is better to use ``outerScope.this`` | ||
instead of passing that info along via Alert's attributes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd delete this. It's true - but I think people might not understand what you mean, and will second guess themselves. Let's focus on showing them the tools they have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
src/TwigComponent/doc/index.rst
Outdated
|
||
.. versionadded:: 2.12 | ||
|
||
The ability to refer to the scope of higher components via the ``outerScope`` variable was added in 2.12. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to the top of the new docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
b57e4a5
to
e618111
Compare
Thanks Bart! |
This PR's is an alternative approach to the problem stated in #863.
Due to the changes proposed in #920, which deals with passing blocks down to nested embedded components, I realized that just being able to access the host (or parent) component isn't sufficient if blocks can be passed down multiple levels deep (as already mentioned in #863 (comment)).
This PR adds a proxy object
Hierarchy
which allows this syntax:Component.php
Resulting in
Adding another layer below Foo, would ask for something like
to still point to Foo's function.
Questions
There's also a
ComputedPropertiesProxy
object. Should we have a cached version of theHierarchy
proxy?NOTE
This PR continues on the changes of #920, because having a hierarchy only makes sense if blocks can be passed down multiple times. So #920 may have to be merged first, but if needed, and the
outerScope.this
syntax is preferred over the syntax of #863, I could push my branch that starts of the main branch instead.Edited: using outerScope variable now instead Hierarchy proxy