Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve callbacks url generation 2 #1651
base: develop
Are you sure you want to change the base?
Improve callbacks url generation 2 #1651
Changes from all commits
8510aa7
eab027a
d29ee2f
5e473de
a80a494
d657e25
4ac3d9e
0437354
64e0c84
a65c2ce
f333c81
e118723
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@ibelar does
$v->canTrigger()
make sense here and any idea how to fix the remaining failing Behat tests?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.
Yes, canTrigger method will check for a reload. Normally, JsCallback should not run during a reload.
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 see why the test is failing. ViewTester reload is missing Callback arguments. Thus when reload is call, the callback method is not run.
I think the reason is that ViewTester is added directly to Modal, while the callback is added to Button
$button->on('click', $vp1Modal->show());
Resulting in
Thus the callback is not part of the Modal render tree... missing its arguments when reload URL is rendered.
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.
any idea how to solve it?
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.
@ibelar the issue is here:
ui/src/JsReload.php
Line 58 in 8014b6c
mergeStickyArgsFromChildView
which allowed to delegate sticky args from it's children.$this->view
which is placed correctly within the Modal render tree, but the Modal is not delegating it's callback sticky args asmergeStickyArgsFromChildView
was removedTo better understand the issue, I have pushed a debug code in 53a0352#diff-0741d95fd53e7a9a50ab8b371b0d621c3eaf3cb460c79dfe5a886deaec0814a9R68 , when you open the test URL, the expected and actual value must be the same.
I belive,
mergeStickyArgsFromChildView
is needed. Or how else should the$this->view->jsUrl(...
in JsReload work?With 29ecd83#diff-c5bb4a733c5ddd3bb0744ef4c29a8fd1e0bd5999fcf06b7f3cfef4c3d1926ce3R640-R668 Behat passes.
Here is a summary of problems to solve: