Skip to content
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 #1646

Merged
merged 26 commits into from
Jul 21, 2021
Merged

Improve callbacks url generation #1646

merged 26 commits into from
Jul 21, 2021

Conversation

ibelar
Copy link
Contributor

@ibelar ibelar commented Jul 6, 2021

  • move back URL method to View class;
  • remove Modal $viewUrl property hack;
  • simplify URL generation for callbacks;

@ibelar ibelar added the RTM label Jul 6, 2021
@ibelar ibelar requested a review from mvorisek July 6, 2021 20:44
@ibelar ibelar removed the RTM label Jul 7, 2021
@ibelar ibelar marked this pull request as draft July 7, 2021 13:16
src/Wizard.php Outdated Show resolved Hide resolved
@ibelar ibelar marked this pull request as ready for review July 8, 2021 13:39
@ibelar ibelar added the RTM label Jul 8, 2021
@ibelar ibelar requested a review from mvorisek July 19, 2021 13:14
src/Callback.php Outdated Show resolved Hide resolved
@mvorisek mvorisek merged commit f85a5ef into develop Jul 21, 2021
@mvorisek mvorisek deleted the feature/callback-url branch July 21, 2021 21:26
@ibelar
Copy link
Contributor Author

ibelar commented Jul 21, 2021

Why did you put back global stickyGet to the callback? V2 Url as discuss above will contains the callback arguments which we want to prevent right? Seem like the test is passing no matter what....not sure why but it need to be fixed.

My manual test shows that when Callback is run; V2 URL will contain the callback argument.

$app->catch_runaway_callbacks = false;

$b1 = Button::addTo($app)->set('trigger callback');

$v1 = \Atk4\Ui\View::addTo($app);
$cb = \Atk4\Ui\Callback::addTo($v1);

$cb->set(function () use ($v1, $cb) {
    $v3 = \Atk4\Ui\View::addTo($v1);
    $v3->set($cb->getUrl());
    \Atk4\Ui\View::addTo($v3)->set($v3->url());
});

$v2 = \Atk4\Ui\View::addTo($v1);
$v2->set('v2 url: ' . $v2->url());

$b1->link($cb->getUrl());

When using include running callback V2 URL was correct.

@mvorisek
Copy link
Member

I think we should handle collecting the trigger names differently, eg. collect them when building the URL, eg. traversing up the render tree/parents.

The test passes, please fix and open a new draft PR. I can look at the better impl. then.

@ibelar
Copy link
Contributor Author

ibelar commented Jul 22, 2021 via email

@mvorisek mvorisek restored the feature/callback-url branch July 22, 2021 00:24
@mvorisek mvorisek deleted the feature/callback-url branch July 22, 2021 00:27
@mvorisek
Copy link
Member

Can you revert to the state it was prior to your changes? I will fix the test then. Envoyé de mon iPhone

Le 21 juill. 2021 à 19:07, Michael Voříšek @.***> a écrit :  I think we should handle collecting the trigger names differently, eg. collect them when building the URL, eg. traversing up the render tree/parents. The test passes, please fix and open a new draft PR. I can look at the better impl. then. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

here you go #1651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants