Skip to content
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

Docs: Clarify PHP Usage #1418

Closed
wants to merge 1 commit into from
Closed

Conversation

lukeapage
Copy link
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
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
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
Contributor Author

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

@scottgonzalez
Copy link
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.

* [Node.js](http://nodejs.org/) (includes NPM, necessary for the next step)
* Grunt (install with: `npm install -g grunt`)
* Get [Node.js](http://nodejs.org/) (includes NPM, necessary for the next step)
* Install Grunt cli (install with: `npm install -g grunt`)
Copy link
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
Member

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

@lukeapage
Copy link
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.


jQuery UI uses Node.js & Grunt to automate the building and validation of source code.
If you are contributing changes you will need a fork of jquery (see [Getting the Source](#environment-getting-the-source)). If you just want the source code you could clone jquery ui directly:
Copy link
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
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
Member

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


You can also run the unit tests inside phantomjs by [setting up your environment](CONTRIBUTING.md#user-content-environment-recommended-setup).

## Building jQuery UI

jQuery UI uses the [Grunt](https://github.com/gruntjs/grunt) build system.
Copy link
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
Contributor Author

tweaks done

@scottgonzalez
Copy link
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