Skip to content

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

Closed
wants to merge 4 commits into from

Conversation

togakangaroo
Copy link
Contributor

Documents changes in PR 1999

@togakangaroo
Copy link
Contributor Author

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>
Copy link
Member

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. 😄

Copy link
Member

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 to POST.

Copy link
Member

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.

Copy link
Member

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.

@arthurvr
Copy link
Member

arthurvr commented Jan 9, 2015

For the reference, issue is #620.


IMHO I'd like to see an example using the new technique in here.

Unfortunately I couldn't get the vagrant VM running so this is untested but I think it's alright?

@togakangaroo Looks alright.

Documents changes in PR 1999

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>
Copy link
Member

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.

@dmethvin
Copy link
Member

dmethvin commented Jan 9, 2015

@arthurvr do you have a working docs setup? Can you help @togakangaroo by pulling down this PR and seeing if it passes XMLLint checks?

@arthurvr
Copy link
Member

arthurvr commented Jan 9, 2015

@dmethvin That's what I meant with "looks alright". Everything passes and is building/deploying nicely.

@togakangaroo
Copy link
Contributor Author

Alright, fixed up the points above. @dmethvin regarding jQuery.ajax vs $.ajaxSetup, sounds like something that is bigger than this PR, right? Or do you want me to take a look at it here?

@dmethvin
Copy link
Member

Let's just be consistent here for both. Based on the jQuery.ajax entry I think we should use $.get, $.ajaxSetup, etc. inside paragraphs.

<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>
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect. Landing right now.

@arthurvr arthurvr closed this in c24d60b Jan 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants