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
Christopher McCulloh edited this page Feb 11, 2015
·
13 revisions
- Assign the PR to yourself.
- 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.
- Confirm control actually works (using either index.html or dev.html.example), paying special attention to whether targeted bugfix and/or new feature works as expected. Be sure to use BrowserStack or other means to check supported browsers.
- Check inside the test folder and examine the control's qUnit tests, making sure any new features have appropriate unit tests.
- Run
grunt serve
in the terminal. 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 there as well. (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 and/or additional unit tests if any issues are found.
- If there are an unnecessarily large number of 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 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.