Skip to content

ajax: documented new post and get config object interfaces#622

Closed
togakangaroo wants to merge 4 commits into
jquery:masterfrom
surgeforward:master
Closed

ajax: documented new post and get config object interfaces#622
togakangaroo wants to merge 4 commits into
jquery:masterfrom
surgeforward:master

Conversation

@togakangaroo
Copy link
Copy Markdown
Contributor

Documents changes in PR 1999

@togakangaroo
Copy link
Copy Markdown
Contributor Author

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

Comment thread entries/jQuery.get.xml Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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! 😄

Comment thread entries/jQuery.get.xml Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread entries/jQuery.get.xml
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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