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

Update JqueryEngineHelper.php #3922

Closed
wants to merge 2 commits into from
Closed

Update JqueryEngineHelper.php #3922

wants to merge 2 commits into from

Conversation

Tzaoh
Copy link
Contributor

@Tzaoh Tzaoh commented Jul 10, 2014

I hope this one is correct.
Because this a feature request it should go on cakephp 2.6 branch.

I would like being able to specify more then one id to be called when updated after an ajax request.
I know it is also possible accomplish this, wrapping all tags with and external tag, but I want to have both options.

Due compatibility with apps already coded it checks if parameter is not an array

I was asked to build a Test case to back my change, so instead of using an string to indicate the following call that we want to update just the tag whose have the specified id we do this:

<?=
    $this->Js->submit(
        __('Update'),
        array(
            'async'             =>  true,
            'class'             =>  'btn btn-default',
            'div'               =>  array(
                'class'         =>  'form-group'
            ),
            'method'            =>  'get',
            'update'            =>  array('#id_1', '#id_2'),
            'url' => array(
                'controller'    =>  'players',
                'action'        =>  'get_player/' . $name
            )
        )
    );
?>

Please let me know if I should improve any part of my pull. Thanks for your patience.

I would like being able to specify more then one id to be called when updated after an ajax request.
I know it is also possible accomplish this, wrapping all tags with and external tag, but I want to have both options.

Due compatibility with apps already coded it checks if parameter is not an array
@dereuromark dereuromark added this to the 2.6.0 milestone Jul 10, 2014
@@ -261,7 +261,13 @@ public function request($url, $options = array()) {
if (isset($options['success']) && !empty($options['success'])) {
$success .= $options['success'];
}
$success .= $this->jQueryObject . '("' . $options['update'] . '").html(data);';
if (is_array($options['update'])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you cast, you can avoid the if/else and the is_array() check completely:

$elements = (array)$options['update'];
foreach (...) {}

done :)

@Tzaoh
Copy link
Contributor Author

Tzaoh commented Jul 10, 2014

You are right, much better this way.
Thank you!

@Tzaoh Tzaoh closed this Jul 10, 2014
@dereuromark
Copy link
Member

Why did you close? You can directly push your changes here on the same branch. This PR will then be updated automatically.

@bcrowe
Copy link
Contributor

bcrowe commented Jul 10, 2014

@Tzaoh To do what @dereuromark mentioned, go to your fork on GitHub, select your patch-1 branch from the dropdown, and continue to use the GitHub online editor. This will continue to add commits to this PR.

@Tzaoh Tzaoh reopened this Jul 10, 2014
@Tzaoh
Copy link
Contributor Author

Tzaoh commented Jul 10, 2014

Oh sorry, he wrote "done :)" and i thought he will commit it.
I try to do it now.
Sorry for your headaches guys ^^

Most optimized code
$elements = (array)$options['update'];
foreach ($elements as $elem) {
$success .= $this->jQueryObject . '("' . $elem . '").html(data);';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get a test for whatever this is fixing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @markstory , the modification of the last patch is because the first version of it was less optimized than casting as @dereuromark pointed. This way is more more clear and efficient.
I am not sure if your message is to me. If that is the case how should I send you a test case which only involves "optimization"?

Thank you

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is still the same as in your previous PR: there is no test case that goes along your changes.
To make things clear: A test with an array of "update"s ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, @dereuromark , is not enough with the code I posted on first comment?
Should I attach a ctp file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe one of us can add the test for 'update' => array('#id_1', '#id_2'), if he can't do it.

@ADmad
Copy link
Member

ADmad commented Jul 10, 2014

Why are we adding features to an engine when JsHelper and all its engines are already removed in 3.0? :) Imo we should encourage people to directly write js code.

@dereuromark
Copy link
Member

For the live line of 2.x this is still an included (and used) class - no matter how deprecated.
If someone wants to provide additional functionality or a bugfix this should probably not be ignored then. Especially if he is able to produce it on its own.

@Tzaoh He has a point, though, you should start using plain JS or write your own plugin solutions for this in the future if possible.

hmic pushed a commit to hmic/cakephp that referenced this pull request Aug 24, 2014
@dereuromark
Copy link
Member

@Tzaoh Would you be able to modify your PR with the suggested changes (missing test case should be added for how to need the array here)?
Then we can fit it into the 2.6 release.

@markstory
Copy link
Member

Closing as its been quite a while with no updates. @Tzaoh if you find the time to add the test cases, let us know and we can get the changes merged in.

@markstory markstory closed this Oct 2, 2014
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.

5 participants