Skip to content

Docs: Clarify PHP Usage#1418

Closed
lukeapage wants to merge 1 commit into
jquery:masterfrom
lukeapage:running-tests
Closed

Docs: Clarify PHP Usage#1418
lukeapage wants to merge 1 commit into
jquery:masterfrom
lukeapage:running-tests

Conversation

@lukeapage
Copy link
Copy Markdown
Contributor

Clarify that PHP is not required for testing and add a link to the CONTRIBUTING page.

I was a bit confused by the Readme and it seems to be out of date. Sorry if I am wrong about the tests not requiring PHP, but they seem to run without it and its alot less hassle to not need a PHP server.

@scottgonzalez
Copy link
Copy Markdown
Member

I forget if we ever actually required PHP for unit tests, but I believe you're correct that we don't require them for any tests now.

I still feel pretty strongly that a first time contributor with a small patch (which is a decent portion of our contributors) shouldn't need to install node. Perhaps we should have both minimal requirements and recommended setups documented. Ideally you run grunt to do full linting and testing, but many contributors can get away with just running unit tests in their browsers.

@lukeapage
Copy link
Copy Markdown
Contributor Author

I think that would make things clearer - I can change to that.

re: the contributors section, I thought that was a bit unclear - there are loads of links and the ones in the readme (I didn't notice CONTRIBUTING at first) point to the wiki page - where some of the links e.g. Commit Message Style Guide go to invalid pages.

Perhaps it is better someone experienced re-writes or looks at those paragraphs - but I could certainly do that with some guidance - otherwise I'll just clean up the testing requirements.

@lukeapage
Copy link
Copy Markdown
Contributor Author

Okay, I've shuffled things around a bit, I think it is a further improvement.

@scottgonzalez
Copy link
Copy Markdown
Member

re: the contributors section, I thought that was a bit unclear - there are loads of links and the ones in the readme (I didn't notice CONTRIBUTING at first) point to the wiki page - where some of the links e.g. Commit Message Style Guide go to invalid pages.

Thanks for pointing that out, I'll go through and clean that up. We've done a lot of work to try to consolidate this information on the new contribute.jquery.org site. I guess there are still a few older pages that have links that haven't been updated to point to the new content.

Comment thread CONTRIBUTING.md 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.

npm install -g grunt-cli

@scottgonzalez
Copy link
Copy Markdown
Member

Overall, this looks good to me. I'd like to wait for some others to chime in though.

@lukeapage
Copy link
Copy Markdown
Contributor Author

I've adjusted based on comments and also made a few further change - Build a Local Copy of jQuery UI in contributing actually does not tell you how to build a local copy of jquery - it contained some information on how to fork and some information on how to run the tests. I've moved those into their own sections. Also made the title casings consistent and improved the duplication/organisation of information.

I understand this is a little controversial and that everyone will have their own opinion on the best wording, but I am sure this is better than it is currently, so will just see if anyone else has any feedback.

Comment thread CONTRIBUTING.md 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.

a fork of jquery-ui

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.

"clone jquery ui directly"

It's always either jquery-ui (the repo) or jQuery UI (the project).

@scottgonzalez
Copy link
Copy Markdown
Member

Thanks for the updates. I left comments for a few small tweaks, but this looks great.

Comment thread README.md 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.

While we're making changes, we should probably update this to link to http://gruntjs.com/ instead.

Clarify that PHP is not required for testing, add a link to the
CONTRIBUTING page and tidy up.
@lukeapage
Copy link
Copy Markdown
Contributor Author

tweaks done

@scottgonzalez
Copy link
Copy Markdown
Member

Thanks.

scottgonzalez pushed a commit that referenced this pull request Feb 9, 2015
Clarify that PHP is not required for testing, add a link to the
CONTRIBUTING page and tidy up.

Closes gh-1418
(cherry picked from commit 8cc636d)
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.

2 participants