PR Checklist - SeedCompany/cord-docs GitHub Wiki

General

  1. Sometimes a PR will have multiple places that need to be changed, but there will be only one comment. When responding to a comment ensure you have reviewed the rest of the PR for applicability.
  2. When comparing date objects don't use the identity comparison === on the object. Use the number representation of the object (epoch millis, etc).
  3. Variable/method names should be descriptive enough so as not to be confused with other variables/methods in the codebase. Think like someone who is not familiar with the code base. Be as descriptive as necessary when naming.
  4. Mutations (not GQL) should be considered smells that warrant further inspection. Consider more functional ways when working with objects.
    Examples include:
    • Using let to declare and modify variables (though there are some use-cases for this like try/catch)
    • Assigning properties to an existing object object.foo = 'now bar'. Prefer creating a new object and spreading the old in { ...object, foo: 'now bar' }. For cases where you need to iteratively "build"/assign changes, ensure it's done on a new object instead of modifying the existing one.
  5. index files should only contain export statements.

Front End

  1. Creating a new small component: If a new component is similar to an existing component and only adds a few properties, ensure that most props are passed through to wrapping component to improve reusability.
  2. Event.preventDefault(): Interrupting natural event handling is sometimes a smell. Consider other methods so that your component doesn't need to interrupt an event handler.
  3. Ensure all css styles are actually needed. Comments describing why are helpful.
  4. Ensure the new component matches the designs, or a comment on the need for a stop-gap version exists (usually a time-constraint).

React

  • The key attribute is only needed on dynamic lists (i.e. Array.map).
  • You probably don't need useEffect.

CSS

  • Is this a magic number/color/font?
    • We want to limit these.
    • Certain values are good:
      • 1-8 spacing
      • palette colors
      • named typography variants
    • If we break these, we should comment why.
  • Does this actually change layout? or is it superficial?
  • Is it the smallest/cleanest way to convey this layout/styling?
  • Should this be in the theme or another component abstraction instead?
  • Is this component independent of external layout or does it assume contribute to it?
  • Are "surprising"/"bad practice" properties commented communicating why or the intent of this choice?
  • Does this look good on all screen sizes?
  • New code should not use StyledComponents or makeStyles

HTML

  • Is this element only for styling that could be combined (easily) into its child?

    <div class="c1">
    <div class="c2">
  	  <div class="c3" />
    </div>
</div>
    <div class="c1 c2 c3" />

    Practically that could look like

    <Box sx={{ m: 1 }}>
  <MyCard />
</Box>

    which should be

    <MyCard sx={{ m: 1 }} />

  • Reusable components should accept styling properties (sx, className)

API

  1. Unnecessary dependency: If a service has hard coded reference to another service, ensure that such are actually required. If its possible to abstract a relationship between two services, a hard-coded reference is unnecessary.
  2. Over-simplification: Sometimes a bug fix where the solution is to simplify an algorithm results in an unintended second-order consequence. When you introduce a new algorithm/process/query, be sure to think about the down range effects.

Designs

  1. Ensure the components in a design document are internally consistent. That the design for a component is the same in every instance that it exists in the document.
⚠️ **GitHub.com Fallback** ⚠️