Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

Latest commit

 

History

History
661 lines (524 loc) · 25.1 KB

CONTRIBUTING.md

File metadata and controls

661 lines (524 loc) · 25.1 KB
Raw

Welcome!

We're so glad you're thinking about contributing to an 18F open source project! If you're unsure or afraid of anything, just ask or submit the issue or pull request anyways. The worst that can happen is that you'll be politely asked to change something. We appreciate any sort of contribution, and don't want a wall of rules to get in the way of that.

Disclaimer: Everything in this guide is not set in stone and subject to change based on changing project requirements and tools.

Before contributing, we encourage you to read our CONTRIBUTING policy (you are here), our LICENSE, and our README, all of which should be in this repository. If you have any questions, or want to read more about our underlying policies, you can consult the 18F Open Source Policy GitHub repository at https://github.com/18f/open-source-policy, or just shoot us an email/official government letterhead note to [email protected].

High-level roadmap

The cloud.gov dashboard is a unit-tested React single-page application. Its previous version was an Angular app and is available at the deprecated branch.

The work on the cloud.gov front-end fits into a higher-level roadmap for all of cloud.gov. For now, this is defined quarterly in a PI planning, documented in favro.

Workflow

We track work and progress through Favro which is an agile board detailing the state of work for the dashboard and other customer facing projects.

Board workflow

  • Stories or ideas for features can start in backlog or icebox.

Criteria for moving through columns

The criteria for moving a card through the columns is in the main cloud.gov product repo: cloud.gov Delivery Process. Some aspects that differ or extend for that process as related to the cloud.gov front end:

Awaiting acceptance
  • Have all new files and newly touched files linted (new files can skip linting if under tight deadline)
  • Have all previous unit tests running without error.
  • Non-JSX code covered in units tests.
  • Is deployed on a demo, staging or live site so other team members can see/use it.
Done
  • Stakeholders see and approve the work as meeting acceptance criteria.
  • If the work has a visual aspect, post a screenshot attached for later documentation/announcement/demo purposes.
Definition of done for a feature

A feature is a higher-level epic that will encompass multiple smaller units of work. An example would be "Route panel" or "Service panel on app page".

  • Stakeholders see and approve the work as meeting acceptance criteria.
  • Is deployed to production, dashboard.cloud.gov site.
  • All appropriate shared global styling is in cg-style rather then cg-dashboard or other repos.
  • All potential errors are handled correctly.
  • If the feature uses new data from the Cloud Foundry API (new methods added to cf_api.js), ensure they are mocked in the testing server.
  • Open an issue or PR in cg-docs to document the new feature.
  • A product owner or team member will check a story waiting acceptance and put it into "done" if it meets the criteria.
  • The feature has been usability tested with at least two current or potential cloud.gov users.

For more information, see the high-level cloud.gov repository and delivery process.

Code standards

Technology

Workflow

  • Add the "ready for review" label when the code is ready to be reviewed by another team member.
    • Work-in-progress PRs are allowed. Be sure to tag the review with "ready for review" when it's ready though.
  • As another team member, review the code and ensure it conforms to the coding standards and exit criteria
    • PR's do not need to be assigned due to small team size
  • When it is reviewed and ready to be merged, use Github's Code Review feature to approve the pull request.
  • Any team member (code author or otherwise) can merge the code once it has an approved review.
    • Updates on PRs in the repo will be posted in the #cloud-gov-nav-news Slack channel
  • It's fine to merge code that isn't "feature complete." The demo branch is not currently in use, so it is fine to force push the branch for "in progress work", e.g. git push -f origin HEAD:demo.
  • We're currently not focusing on acceptance tests right now due to the tests not being easily repeatable and having a clean data state. If a change breaks an acceptance test, spend 10 minutes trying to fix it before disabling the test. Do not write new acceptance tests.

Other Git standards

  • Squashing commits is allowed but discouraged, except in rare instances.
  • The team prefers rebasing over merging, though we use GitHub to close out pull requests. This means that PRs will be merged, but if you're refreshing a local branch make sure to use rebase. For example, if you want to update your new-feature branch to reflect the most recent changes on GitHub use git pull --rebase origin new-feature.

Branches

  • Open branches off main repo due to Circle CI env var problem. For now, remember to branch off of the master branch.
  • Name your branch with your initials first. For example, ms-feature.
  • Include a short description of the feature that's being developed after your initial.

Commit message

In general, commit messages can be written in whatever way the author decides, but here are some guidelines:

  • Focus on the "why" rather then the "what".
  • Have the first line be the "what".
  • Discuss the "why" in more detail on subsequent lines.

Coding style

Formatting

Prettier is used to automatically format a range of files (particularly .css, .js, .jsx, .json and .md). See prettier's docs for how to enable automatic formatting on save for your editor.

If it's necessary to manually run prettier, you can use the package script:

npm run format

Note: it's highly recommended to format on save, so the package script should generally not need to be used.

Linting

The code base includes linting configurations and tools, but is currently not fully linted. This means that there's an "opt-in" policy to linting: you decide when to add a file to linting.

  • Theres a lint ignore file, .eslintignore with all non-linted files.
  • If you touch another file that isn't linted yet, you should generally fix it and remove it from the lint ignore.
    • Unless under strict time constraints.
  • No new files should be added to the lint ignore. Consequently, all new files should be linted.
  • Linting will be run before tests run, so will fail the tests if files are not linted.
  • Additionally, linting should always
    • fail on CI
    • should not stop watching commands
    • should not fail builds
    • should fail test runs

Documentation

Documentation is not a focus right now.

Testing

All pull requests are tested with CircleCI and CodeClimate. To run the CodeClimate tests locally, you can use the CodeClimate CLI with Docker by invoking: codeclimate analyze. Only the nodesecurity tests will block acceptance, which you can isolate by running: codeclimate analyze -e nodesecurity.

The CirleCI tests are described in the circle.yml stanza for test:, but in short

  • FrontEnd tests: If you followed the Manual Setup, tests are invoked with npm run test. If you're using the recommended Docker setup, then you'd invoke: 
    docker-compose up -d frontend
docker-compose exec frontend bash -c "npm run test"
    The Docker image consol/ubuntu-xfce-vnc may encounter a ChromeDriver bug that results in errors like the one below: 
    Error: An unknown server-side error occurred while processing the command.
    at cookie("DELETE", "testing_user_role") - deleteCookie.js
    Failures related to these errors can be ignored, as they're not present in the CircleCI tests.
  • Backend Tests: The Go server test suites are invoked with ./codecheck.sh.

Dependency management

  • The only language level dependencies should be nodejs and go.
  • Front end dependencies (as in package.json) should be pinned to minor versions and below under most circumstances.
  • Front end dependencies should be shrink-wrapped with npm.
  • Every month, front end dependencies will be upgraded to most recent patch or minor version as stipulated in package.json.
  • React, Babel, Webpack, ImmutableJS, and other major libraries should upgraded to major versions when there's time to test the changes.

How to upgrade dependencies

To upgrade all dependencies to version stipulated in package.json

  • Ensure cloudgov-style or other dependencies are unlinked by running npm unlink cloudgov-style.
  • Dependencies you unlink may need to be manually replaced with, say, npm install --save cloudgov-style
  • Remove npm-shrinkwrap.json.
  • Use npm update --save --saveDev to update all dependencies.
  • Make sure it works (run linting, tests, and acceptance tests).
  • Add back the shrinkwrap with npm shrinkwrap --dev and commit it.

To update a single package to a specific version

  • Ensure cloudgov-style or other dependencies are unlinked by running npm unlink cloudgov-style.
  • Run npm install --save[Dev] {package name}@{version} where package name is the name of the package to update and version is the version you want to update to.
  • If there are errors related to peer dependencies, continue installing them in the same fashion.
  • If there are no errors, both package.json and npm-shrinkwrap.json should have changes that can be committed.

Code review

  • When doing code reviews, the reviewer should pull down the code and test on their local computer. This is because the staging site is not often used, meaning bugs could be present for long amounts of time.
    • If a code change is very simple, or is not a code change (but maybe a document change) it doesn't need to be pulled down.
  • We can give other members of 18F the public URL to the staging site.

Device support

  • Browser support is all major browsers and Internet Explorer 10 and up.
  • Pages and views should work on different device screen-sizes, and should work as specified in design mockups.
    • If making a feature work on mobile is time-consuming, and there isn't an official design for the mobile view of the feature yet, the mobile view can be held off until a design becomes available.
  • Support for browsers not running javascript should be attempted if it's easy, but not a major focus
    • Eventually the react code base will allow server-side, isomorphic rendering for both performance and support reasons. This means code should attempt to ensure things work without it when it's not detrimental to project timeline to do so.
  • CSS in component files should opt to use CSS Modules first, and will use normal CSS class architecture after.

Working with React/Flux

The dashboard uses the Flux architecture and renders HTML with React. The Flux implementation is mostly vanilla JS with a Flux dispatcher, as opposed to using a tool such as Redux.

Action creators

Action creators are simple functions which always dispatch an action of a certain type with certain data through the central dispatcher and perform a API request if the action requires it. These actions are listened to by just the stores, which will usually modify their data when an certain action happens.

Here are some basic rules to work with action creators successfully:

  • An action creator should usually have a matching store, i.e. UserStore -> UserActions.

  • An action creator should have a corresponding constant for the action.

  • Action creator methods should generally be named as {verb}{noun} and use the appropriate tense based on whether the action is going to happen vs it has happened

    // Good
fetchSpaces();
receivedSpaces();
addUser();
addedUser();

// Bad
spacesFetch();

  • Each action creator (each method in *_actions.js) should dispatch only a single action.

    // Bad
addedUser(user) {
  AppDispatcher.handleServerAction({ type, user});
  AppDispatcher.handleServerAction({ type2, user});
}

  • Each action creator should return a Promise.

    // Good
receivedSpaces(spaces) {
  return Promise.resolve(spaces);
}

  • There should not be more than one action with the same data.

  • Any XHR calls should happen within the fetch action creators which should return a promise of the request.

  • Any fetch action should call the appropriate receive/success/error action on the resolve of the fetch's promise.

    // Good
fetchUser(guid) {
  return cfApi.fetchUser(guid).then((user) => userActions.receivedUser(user));
}

  • Each async action should have a fetch, success, and error sub-action (e.g. TOGGLE, TOGGLE_SUCCESS, and TOGGLE_ERROR).

Stores

Stores are meant to hold all the data of the application such as the Cloud Foundry entities such as organizations, spaces, apps, etc as well as UI data such as loading states, current pages, and current user info. They use Immutable.js as their main data structure to limit accidental manipulation of the data. They bind to certain actions and change their data. When they change their data, they emit a changed which is listened to by the components that require their data, which in turn render.

Here are some basic rules to work with stores successfully:

  • Stores should not call action creators.
  • The data in stores should be an Immutable.List.
// Good
export class OrgStore extends BaseStore {
  constructor() {
    super();

    this.currentOrgGUID = null;
    this.isFetchingOrg = false;
  }

  get loading() {
    return this.isFetchingOrg || this.isFetchingAll;
  }
}

  • Stores should generally not access their storeData directly but should attempt to use the data manipulation functions in the BaseStore, such as push, merge, mergeAll, etc.

    // Good
case orgActions.ORGS.RECEIVED: {
  this.mergeAll('guid', action.orgs);
}

// Bad
case orgActions.ORGS.RECEIVED: {
  this.storeData = new Immutable.List(this.storeData, action.orgs);
}

  • Show preference for emitting a change as the performance hit is worth reducing bugs due to changes not being reflected in the UI because the change wasn't emitted. This should always be the case when something like loading state changed.

    // Good
this.isFetchingAll = false;
this.mergeMany("guid", updates, () => {});
this.emitChange();

// Bad
this.isFetchingAll = false;
this.mergeMany("guid", updates, changed => {
  if (changed) this.emitChange();
});

  • Stores should generally avoid doing async work, including making any calls to the API, and setTimeout. The only case where it's easier for the store to make API calls is when a store's loading state depends on more then one data request, in which case, the BaseStore.load method should be used. See anti-patterns for more information

When to create a store

Creating a new store should generally be avoided but there are a few circumstances where it makes sense:

  • There isn't already a store for a certain Cloud Foundry Entity, such as an org, app, space (these already exist).
  • There's a need for data to be shared across the whole application, such as global notifications and errors.

Components

Components are React JSX components that render the HTML UI of the application. Components subscribe to stores that have data they care about or have properties that are passed down from the component that rendered them. Components call action creators when things happen in the UI, such as a user clicks a button to add an entity.

Here are some basic rules to work with components successfully:

  • Always set the component's state with the setState method (unless in the constructor). Never set state in the render() method.

    // Good
this.setState({ ...data });

// Bad
this.state = { ...data };

  • Due to the flux architecture, components should rarely set their own state, but should receive all their state from their stores. Rather then setting state when something happens, components should call action creators which will change the store data.

    // Good
onHandleClick() {
  userActions.addUser();
  // Eventually state will be set from stores.
}

// Bad
onHandleClick() {
  this.setState({ loading: true });
}

  • Break up long render methods by putting some of the HTML UI in other methods.

    // Good
getErrorMessage() {
  return <ErrorMessage></ErrorMessage>
}

render() {
  const error = this.getErrorMessage();
}

  • Be sure to bind methods to this that are called from click or DOM events in the constructor.

    // Good
this.handleOverviewClick = this.handleOverviewClick.bind(this);

  • For UI that is only rendered conditionally, set the UI's variable with an empty let statement, as React will not render undefined items, but will also not crash.

    // Good
let errorMessage;
if (this.state.error) {
  errorMessage = <Error>{ this.state.error }</Error>
}
...

return (<div>
  { errorMessage}
  { otherStuff }
</div>);

// Bad
if (condition) {
  conditionalContent = <div></div>;
} else {
  conditionalContent = ''; // Not required
}

  • Keep the propTypes and defaultProps as consts at the top of the file directly before the component class definition, and set the consts at the bottom.

    // Good
const propTypes = { things: PropTypes.array };
const defaultProps = { things: [] };

class Things extends React.Component {
...
}

Things.propTypes = propTypes;
Things.defaultProps = defaultProps;

  • If UI displaying is dependent on a certain amount of time, this should be set in the component rather then the store, see Loading component as an example.

  • If a component's prop types is technically an enum, use oneOf rather then the underlying data structure.

    // Good
const propTypes = { status: PropTypes.oneOf(['red', 'green', 'yellow']);

// Bad
const propTypes = { status: PropTypes.string };

  • Components should usually have either state or props, but usually not both. See Container component.

    • The only time it should have both when there's a prop for a configurable part of the UI.

Patterns

UI actions

When a UI is supposed to do something based on something a user did, such as clicking a a dropdown to display what's within, these actions should go through the dispatcher as an action.

Container component

The general pattern for different UIs on the app is to fetch data for Cloud Foundry entities, and then render React components with the data. To organize around this, container components should get and keep the state of the data and pass down this data through various UI based components through props. This means UIs will have one container (which is usually but not always linked to a page, like org, space, etc) and all other components will just have props and will have all data passed down through them.

There's no perfect guidance on what should be a container component vs a props component, besides that almost all base pages are a container component. A good way to determine is to see what data various UIs require and then separate containers and prop containers based on this.

Anti-patterns

Setting state in component rather then through actions

Besides pure UI components, no components should set state within themselves without the use of an action. This is due to keeping both UI actions the users take, and server actions when data comes in from the server to move through the app in the same way, through the dispatcher.

Instead, use ui actions.

Doing async work in the stores

This pattern is slowly being phased out of multiple stores, so will be seen many places. For new stores, all async work should be done in the action creator functions. For example, for a fetch action, the fetch action should be dispatched and then the API call should be made.

When async code is in stores, it should be migrated to action creators.

Analytics

Both client-side routed page views and all client-side events are logged in google analytics.

  • Different client-side urls, such as /org/{org guid}/spaces/ are logged as page views. This is done through the router.
  • Any action called is logged as a Google Analytics event. This is done from the App Dispatcher.
  • There's a utility file for all analytics related functionality, util/analytics.js

Google analytics dashboard

There's no distinction from the dashboard and any of the other cloud.gov site's in the GA dashboard. To help with this, there's a shared GA segment representing just the dashboard production traffic. If you have access to the cloud.gov GA, it's available with a shared link. There's also a dashboard GA dashboard, which doesn't have a shareable link but should be in the "Customization" -> "Dashboard" section.

Performance

Performance, as in speed of the site as perceived to users, has been briefly researched and tracked. The main web performance metrics tracked are Speed Index, input latency, time to interactive, total page weight, and number of DOM nodes. These metrics were tracked for five comparison sites for cloud.gov and documented in a google sheet, GSA only. These comparison stats were used to determine performance goals (ideal numbers to reach over time) and budgets (limits placed on the team to stop performance from degrading).

Performance is tracked as part of the CI build process, and the build fails if performance goes over the budgets. Any library added that's total file size is above 25kb should be evaluated for performance affect. Update 07/05: Currently the performance is not tracked as part of the CI build due to memory and resource limits on CircleCI, but should be added back later when these issues are resolved.

Onboarding checklist

Complete the onboarding tasks for the whole cloud.gov team and for the Customer theme.

Public domain

This project is in the public domain within the United States, and copyright and related rights in the work worldwide are waived through the CC0 1.0 Universal public domain dedication.

All contributions to this project will be released under the CC0 dedication. By submitting a pull request, you are agreeing to comply with this waiver of copyright interest.