[LiveComponent] add LiveProp name to modifier function #2652
Conversation
carsonbot
added
Feature
jannes-io
force-pushed
the
modifier-prop-name
branch
2 times, most recently
from
631af7c to
0a949c8
Compare
src/LiveComponent/doc/index.rst
Outdated
| @@ -2617,7 +2617,9 @@ definition:: | |||
|
|
|||
| Then the ``query`` value will appear in the URL like ``https://my.domain/search?q=my+query+string``. | |||
|
|
|||
| If you need to change the parameter name on a specific page, you can leverage the :ref:`modifier <modifier>` option:: | |||
| If you need to change the parameter name on a specific page, you can leverage the :ref:`modifier <modifier>` option | |||
There was a problem hiding this comment.
A couple of remarks here (unrelated to wether we will move forward with this PR or not :) )
First of all: thank you for thinking immediately "documentation". That's a very nice habit to have.
Now for the changes here: I think we should not change the existing documentation, but instead we may add a dedicated paragraph after this part.
The current version does it jobs, covers the feature and how most developers use it, while keeping things small / easy to understand.
Moreover, using a prefix instead of an alias here would offer less possibilities than before, so we should probably not describe this solution in first examples, but only when a prefix could be justified (many identical components in the same page ? still not sure to be honest)
So here I think a dedicated paragraph after this one, for more advanced use case, could be more appropriate.
| @@ -135,7 +135,7 @@ public function withModifier(object $component): self | |||
| throw new \LogicException(\sprintf('Method "%s::%s()" given in LiveProp "modifier" does not exist.', $component::class, $modifier)); | |||
| } | |||
|
|
|||
| $modifiedLiveProp = $component->{$modifier}($this->liveProp); | |||
| $modifiedLiveProp = $component->{$modifier}($this->liveProp, $this->getName()); | |||
There was a problem hiding this comment.
For a change like this, a test would be needed. Especially for things related to query, URL, etc.
(The fact tests do pass after your changes does not proves it does work (nor that our current suite of test is good enough, to be fair))
There was a problem hiding this comment.
Added to the existing test, I don't see the need to make a separate test for just this param unless there are some edge cases that aren't entirely obvious to me at this time.
|
I personally don't have a big issue with removing the internal. Very thoughtful to realize that issue :). Alternatively, if there is something you need on the metadata, we could add it to LiveProp |
|
I don't see a problem to pass the second argument 👍 Passing the metadata is not something I'd love to do, without an absolute need to do so. |
Okidoki, I can do the requested changes to the docs and add some tests to finish off this PR. Could you give me a case where the property name could be different from the name in the metadata? Or where I could find code that could create this side-effect so I can try to reproduce? Just want to understand what the implications might be there.
Agreed 😄 |
jannes-io
force-pushed
the
modifier-prop-name
branch
from
0a949c8 to
d3c8127
Compare
| .. versionadded:: 2.25 | ||
|
|
||
| The property name is passed into the modifier function since LiveComponents 2.25. |
There was a problem hiding this comment.
This'd have to be updated to the actual release version, I just took the next one.
Implements proposal 2 from the examples in the RFC.
Personally I would just pass the entire liveprop metadata object to this function instead of only the name, but it's marked internal so I'm not sure if we should be exposing it to users like that.