Document the 'submit' option in CHtml::clientChange() better. #1530

Merged
merged 4 commits into from Oct 5, 2012

Conversation

3 participants
Contributor

rawtaz commented Oct 4, 2012

The previous documentation was okay but little informative. It's good to know how this option works in terms of existing parent forms or the lack of them, as well as which methods are used.

Member

mdomba commented Oct 4, 2012

I think you complicated a bit with this description and made it a bit difficult to understand... Now it's really not so clear what happens "if it's empty" as you used "non-empty" and "otherwise".

Contributor

rawtaz commented Oct 4, 2012

I understand it can look that way, but the previous description was actually inaccurate and needs to be more elaborative to avoid unexpected results. I did this because I had to read the source code of CHtml::clientChange() and jquery.yii.js to figure out a couple of things about how this worked. My intention here was to make the decription more elaborate so the aforementioned isn't necessary.

There are more things hidden in this feature/setting than is apparent at first. Aside from empty/non-empty affecting the outcome, there's also the parent form/no parent form.

For example, if one is applying 'submit'=>'' on an element that has a parent form, and the parent form has an URL set in its action, then according to the previous description the current URL would be used. But that is not true, it is the form's URL that is used in this case.

This can be confusing if the form uses GET, has an action of ?r=a/b and the current URL is ?r=a/b&foo=bar. One would expect the submit to add the form's data to that URL, creating ?r=a/b&foo=bar&field1=val1&field2=val2, but instead the resulting URL submitted to is the form's URL + the form data, i.e. ?r=a/b&field1=val1&field2=val2.

To be clear, the unexpected and actual result in the above example is perfectly correct (don't change this, and let me know if I should explain why this shouldn't be changed), it's just that the description of submit is inaccurate and too non-elaborative.

Unless there are grammatical errors in the new description I vote that it be merged. I believe it to be accurate and it doesn't explain more than what is needed for one not to have to read source code to understand it.

Member

mdomba commented Oct 4, 2012

I understand all you wrote even before... my comment was not directed in the sense that the addition is not needed but in the sense that it needs further working on it... like using other words... for example using like it was before "if its empty" instead of "otherwise".

Contributor

rawtaz commented Oct 4, 2012

Okay, sorry if I misunderstood your comment. Well, I tried to keep it as short as possible yet well written. I'm sure it can be improved though, but for that I think someone else might need to have suggestions :)

Alright, let me see what I can do, will look at it again when I get a few minutes..

@samdark samdark and 1 other commented on an outdated diff Oct 5, 2012

framework/web/helpers/CHtml.php
@@ -2042,7 +2042,11 @@ public static function listOptions($selection,$listData,&$htmlOptions)
* @param array $htmlOptions HTML attributes which may contain the following special attributes
* specifying the client change behaviors:
* <ul>
- * <li>submit: string, specifies the URL that the button should submit to. If empty, the current requested URL will be used.</li>
+ * <li>submit: string, specifies the URL to submit to. If the current element has a parent form, that form will be
+ * submitted (using its regular method). If 'submit' is a non-empty string it will override the form's URL for the
@samdark

samdark Oct 5, 2012

Owner

using its regular method?

@rawtaz

rawtaz Oct 5, 2012

Contributor

Good pointing out. I was referring to that the method (POST or GET) that is set or default on the form will be used. Will try to address this when i review this after today. Unless someone has a better suggestion already.

@samdark

samdark Oct 5, 2012

Owner

Well, that's expected behavior by default, I think. Is there a real need to mention it?

@rawtaz

rawtaz Oct 5, 2012

Contributor

Probably not. This is one of the things that can be removed I think.

@samdark samdark and 1 other commented on an outdated diff Oct 5, 2012

framework/web/helpers/CHtml.php
@@ -2042,7 +2042,11 @@ public static function listOptions($selection,$listData,&$htmlOptions)
* @param array $htmlOptions HTML attributes which may contain the following special attributes
* specifying the client change behaviors:
* <ul>
- * <li>submit: string, specifies the URL that the button should submit to. If empty, the current requested URL will be used.</li>
+ * <li>submit: string, specifies the URL to submit to. If the current element has a parent form, that form will be
+ * submitted (using its regular method). If 'submit' is a non-empty string it will override the form's URL for the
+ * submit, otherwise the form's URL will be used (or the currently requested URL, if the form has no specific URL
@samdark

samdark Oct 5, 2012

Owner

Using form current URL if no URL specified is a standard browser behavior and we're following it. No need to mention it.

@rawtaz

rawtaz Oct 5, 2012

Contributor

I agree. Changing this right now.

Contributor

rawtaz commented Oct 5, 2012

Okay guys, second attempt at this :)

Member

mdomba commented Oct 5, 2012

The part with "if empty" is missing now...

Contributor

rawtaz commented Oct 5, 2012

How's that?

Member

mdomba commented Oct 5, 2012

Very nice, couldn't say it better

@mdomba mdomba added a commit that referenced this pull request Oct 5, 2012

@mdomba mdomba Merge pull request #1530 from rawtaz/clientChangeSubmitDocFix
Document the 'submit' option in CHtml::clientChange() better.
[ci skip]
be5241d

@mdomba mdomba merged commit be5241d into yiisoft:master Oct 5, 2012

Contributor

rawtaz commented Oct 5, 2012

Thank you both for getting this on the right track again! :)

Member

mdomba commented Oct 7, 2012

We just give directions (that's the easiest part), it's you that have done the hardest part. Thank you for working on this.

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