This repository has been archived by the owner on Feb 12, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1k
PR Review Checklist
Kevin Parkerson edited this page Feb 3, 2015
·
13 revisions
- Check to see PR follows these conventions.
- Ensure PR is linked to appropriate issue via "Fixes" comment in the PR description. Ex:
Fixes #1234
- Pull down repo with changes. Instructions on how to do this will be directly below the comments in the PR "conversations" tab, to the left of the merge button. For more info, view these instructions.
- Check the index page (and/or dev.html page) to confirm control works, paying special attention to whether targeted bugfix and/or new feature works as expected.
- Check inside the test folder and examine the control's qUnit tests, making sure any new features have appropriate unit tests.
- Run
grunt serve
. Check to see that the qUnit tests pass in the terminal, then visithttp://localhost:8000/test/fuelux.html
in browser to confirm qUnit test pass in the browser. (this page can help debug failing unit tests) - Review the code itself via the "files changed" tab and/or your editor. Pay attention to the following items:
- Make sure code adheres largely to the styleguide.
- Event names must be namespaced appropriately. For bound events the naming convention is
{{eventName}}.fu.{{controlName}}
, for triggered events the naming convention is{{eventNamePastTense}}.fu.{{controlName}}
- Check code for quality and efficiency.
- Request changes to the code or additional unit tests if any issues are found.
- If there are over 3 commits, request that the PR creator squash their commits. (instructions on squashing commits with Git and interactive rebasing with Webstorm)
- Ensure PR passes Travis build, re-running once or twice if it fails. Failure could be due to other issues, so determine whether the PR is responsible or not if failure occurs.
- After completing all previous steps, if PR is ready to be merged go ahead and do so using the "merge pull request" button.
- After PR has been merged, wait approximately 5 minutes, then check the edge server page to confirm a recent build timestamp has been created.