-
Notifications
You must be signed in to change notification settings - Fork 264
ajax: documented new post and get config object interfaces #622
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
Conversation
Unfortunately I couldn't get the vagrant VM running so this is untested but I think it's alright? |
@@ -2,6 +2,12 @@ | |||
<entry type="method" name="jQuery.get" return="jqXHR"> | |||
<title>jQuery.get()</title> | |||
<signature> | |||
<added>2.2</added> | |||
<argument name="settings" type="PlainObject" optional="false"> | |||
<desc>A set of key/value pairs that configure the Ajax request. All properties except for <code>url</code> are optional. A default can be set for any option with <a href="/jQuery.ajaxSetup/">$.ajaxSetup()</a>. See <a href="/jquery.ajax/#jQuery-ajax-settings">jQuery.ajax( settings )</a> for a complete list of all settings. Type will automatically be set to <code>POST</code></desc> |
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.
Missing dot at the end of your sentence. 😄
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.
Type will automatically be set to
POST
This seems more clear to me:
The
type
option will automatically be set toPOST
.
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.
Awesome that you caught the $.ajaxSetup
point.
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 think we should be consistent within this paragraph, right now it's $.ajaxSetup
and jQuery.ajax
. I know we use both in docs but whichever is most common elsewhere should be used.
For the reference, issue is #620. IMHO I'd like to see an example using the new technique in here.
@togakangaroo Looks alright.
You might mean jquery/jquery#1995 ;) Thanks for the PR! It's awesome when people doing stuff in core come in here and update the docs! 😄 |
@@ -2,6 +2,12 @@ | |||
<entry type="method" name="jQuery.get" return="jqXHR"> | |||
<title>jQuery.get()</title> | |||
<signature> | |||
<added>2.2</added> |
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.
This will end up being 3.0
, that's our next release.
@arthurvr do you have a working docs setup? Can you help @togakangaroo by pulling down this PR and seeing if it passes XMLLint checks? |
@dmethvin That's what I meant with "looks alright". Everything passes and is building/deploying nicely. |
Alright, fixed up the points above. @dmethvin regarding |
Let's just be consistent here for both. Based on the |
<desc>A set of key/value pairs that configure the Ajax request. All properties except for <code>url</code> are optional. A default can be set for any option with <a href="/jQuery.ajaxSetup/">$.ajaxSetup()</a>. See <a href="/jquery.ajax/#jQuery-ajax-settings">jQuery.ajax( settings )</a> for a complete list of all settings. The type option will automatically be set to <code>POST</code>.</desc> | ||
</argument> | ||
</signature> | ||
<signature> |
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.
Looks like this breaks the indentation. Could you fix that? I'd love to land this PR soon.
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.
Arthur, you could edit this with an --amend
IMO, it's a pretty trivial thing.
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.
Perfect. Landing right now.
Documents changes in PR 1999