# Code review for new components

## Before the code review following steps must be covered

- Code meets the code style specs (`npm run verify-code-style` passes) (git hook verifies this)
- Test coverage is 100% (`npm run test-coverage` is 100%) (git hook verifies this)
- Typescript types are valid (`npm run verify-code` passes) (git hook verifies)
- Components design and behaviour are approved by the designer (deployment branch preview sent to designer who has approved it)
  - TODO: How should this be handled? Should be approved by one of the Gaia core team designers

## The following points are NOT covered by the code review:

- Should the component be accepted into the design system? This needs to be discussed with designers

## Code review checklist

### Folder structure

- Component has an own directory called `YourComponentName` in `src/components`
- Inside the directory, there are two files for your component: `YourComponentName.test.js` and `YourComponentName.tsx`
- Possible style file is named `YourComponentName.module.scss` and placed into component folder
- Any utility files related to only this component (such as `useLinkProvider.ts`) are placed in the component folder
- Component has a story file called `YourComponentName.stories.tsx` under `src/styleguide/[...]/YourComponentName.stories.tsx`
- Component is exposed in `src/index.js` and tested in `src/index.test.js`

### Testing

- Test are readable and describe the behaviour of the component, not just fill the test coverage -rule
- Tests cover all the possible scenarios
- Any callbacks provided in props need to be tested, as long as they are directly handled by this component and not just passed through to another component such as `Element`
- Browser tested in Storybook on at least Chrome, Safari and Firefox (TODO: This should be automated)
  - Play with the props
  - Resize window and see how component reacts
  - Experiment with different kinds of content, incl. missing, small, lots of content
  - Use the component's behavior with both mouse and keyboard
  - Check that there are no errors, warnings or unnecessary messages in the console
  - If it's a new component, check with IE/Edge as well (especially `<Flex />` can be prone to breakage on IE)
- Consider running visual tests locally (`npm run visual-test`) to check for regression when changing existing component
- Consider running accessibility tests against [the kitchen sink](http://localhost:6007/iframe.html?id=kitchen-sink--all-components&viewMode=story) to make sure your changes don't cause obvious accessibility issues

### Documentation

- Components should have type definition documentation with comments and required props
- Component's source code is clear, self explaining and readable. No unhelpful or redundant comments in the code
- Component has a story in Storybook, all the props descriptions are visible there and can be tried out with knobs
  - Overview page has documentation with examples of the component usage and variants
  - Component page has the component with knobs and props descriptions

### Styles

- Element props such `backgroundColor`, `fontWeight`, `boxShadow` etc. are preferred over using CSS classes or inline styles
  - Right: `<Div backgroundColor="purple500" />`
  - Wrong: `<Div className="someClassWithPurpleBackground" />`
- Typography components (BodyText, Heading etc.) are used instead of custom text styles
- Flex component is prefered instead of css
- Component styles are placed in `YourComponentName.module.scss` file under `src/components/YourComponentName`
  - Subcomponents have their own SCSS files if needed
- BEM is used correctly such as `navigation_button__disabled`
- Styles use shared icons, colors, breakpoints, spacing and typography which are specified under `src/shared` instead of custom values
  - Right: `.foo { color: color(purple500); }`
  - Wrong: `.foo { color: #a700ff; }`
- Values specified in pixels are calculated using common `baseline` for spacing whenever possible
- Inline styles must be justified
- `!important` should never be used unless a _very_ good reason requires so

### Code

- Components are not class based
- Typescript definitions are written using helpers `CreateProps` and `CommonProps` when needed
- Code is self explaining, readable, simple and clean
  - Pay attention to naming of functions, variables, props, etc
- Camelcase is used in naming
- Code is not one big chunk but it's split into meaningful blocks, child components and helper functions
- Component code is generic and does not contain application specific business logic
- Component should be stateless
  - If internal state is required, it's extracted into separate container component `YourComponentNameContainer.js`. In this case the component should support option to overwrite the internal state so that it has controlled and uncontrolled version.
- Component does not contain hardcoded strings or numbers but the texts are configurable via props with proper defaults where appropriate
- Avoid breaking changes in existing code
  - Examples: Non-backwards compatible API changes such as removing/renaming props, changing how an existing component renders, removing an existing component, etc
  - Small cosmetic changes such as colors are not breaking, but most layout related changes such as spacing are
  - Any breaking changes must be agreed with the Gaia core team
- Existing components and helpers are reused where possible (eg. `<Backdrop />` component, `isIE()` helper function)
- Similar components have similar APIs
  - Example: `Combobox` and `Autocomplete` both use `<OptionList />` for displaying the options and have an `options` prop with the same shape

### 3rd party libraries

- Adding a new 3rd party library must be agreed with the core team
  - Prefer libraries that have Typescript support
- 3rd party library added in the dependency list in `package.json` and `package-lock.json`
- Make sure there are no known vulnerabilities and an up-to-date version is used

### Type definitions are updated

- Type definitions related to the changed components are updated accordingly in telia-react-component-library.d.ts

### Pull request

- The PR is targeted at the correct branch
  - No breaking changes to `master` if there is a `breaking-change` branch (or similar)