|
| 1 | +# Contributing Code |
| 2 | + |
| 3 | +1. Create a new issue before starting your project so that we can keep track of what you are trying to add/fix. That way, we can also offer suggestions or let you know if there is already an effort in progress. |
| 4 | +2. Fork off this repository. |
| 5 | +3. Create a topic branch for the issue that you are trying to add. |
| 6 | +4. Edit the code in your fork. |
| 7 | +5. Send us a well documented pull request when you are done. |
| 8 | + |
| 9 | +**GitHub pull requests** should meet the following criteria: |
| 10 | + |
| 11 | + - descriptive title |
| 12 | + - brief summary |
| 13 | + - @mention several relevant people to review the code |
| 14 | + - add helpful GitHub comments on lines where you have questions or concerns |
| 15 | + |
| 16 | +We'll review your code, suggest any needed changes, and merge it in. Thank you. |
| 17 | + |
| 18 | +## Concepts and Best Practices |
| 19 | + |
| 20 | +- <a name="approved-slds-patterns" href="#approved-slds-patterns">#</a> This library should include only components which have approved patterns in SLDS. |
| 21 | +- <a name="familiarize" href="#familiarize">#</a> Familiarize yourself with concepts used in the rest of the library. |
| 22 | +- <a name="eslint-all-files-touched" href="#eslint-all-files-touched">#</a> If a file is touched that has outstanding ESlint errors, please fix the ESlint errors first (and in a separate commit). Sometimes special cases require an `eslint-disable` comment for a particular rule and/or line. Please use sparingly. |
| 23 | +- <a name="react-create-class" href="#react-create-class">#</a> `React.createClass` is preferred over ES6 classes and `extend` at this time. |
| 24 | +- <a name="stateful-stateless-components" href="#stateful-stateless-components">#</a> Know how smart/stateful React components [work together](https://gist.github.com/trevordmiller/a7791c11228b48f0366b) with [pure/dumb stateless function components](https://facebook.github.io/react/docs/reusable-components.html#stateless-functions). |
| 25 | +- <a name="stateful-top-level-component" href="#stateful-top-level-component">#</a> It is preferable to only have one stateful top-level class per component in this library. For these top-level components, it’s preferable to leave them stateful (that is, to use `React.createClass`). It's much easier to get the DOM node reference if you need it for such things as measurements. Then, you don't have to go through a lot of hassle to work around not having lifecycle methods. It also allows components to follow the controlled / uncontrolled pattern mentioned below. All sub-components should be stateless and manipulated with props if possible. |
| 26 | + - A Tree should have state. A tree node should not. |
| 27 | + - A Data Table should have state, a Table Column should not. |
| 28 | + - Frequently used items such as badges, pills, buttons or icons should probably not have state. |
| 29 | +- <a name="avoid-mixins" href="#avoid-mixins">#</a> Avoid mixins. Instead, import and use shared code and external libraries as libraries, or use higher-order components. Do not add external dependencies unless absolutely necessary. Consider the "total cost of ownership" of all dependencies. |
| 30 | +- <a name="rest-operators-with-jsx" href="#rest-operators-with-jsx">#</a> Be careful with [rest operators](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters) when passively applying unnamed and unknown props to JSX nodes. This concept allows flexibility to the consuming developer, but is difficult to track for maintainers. If rest operators should be used, be sure to deconstruct each one that is actually needed by the JSX nodes, so that the rest operator only handles "unknown props" passed in from the outside developer. In short, don't utilize any properties in the `...props` object within the component. After using `const { active, className, ...rest } = props;` do not go back to using `this.prop.*` anywhere in the render function. |
| 31 | +- <a name="rest-operators-with-jsx-delete" href="#rest-operators-with-jsx-delete">#</a> If a rest operator is already present in your render function and you need to remove additional props so that they do not get passed to a JSX node, use the rest operator along with `// eslint-disable-line no-unused-vars` to remove the prop from `...rest`. |
| 32 | +- <a name="dimoc" href="#dimoc">#</a> Use the controlled/uncontrolled callback/prop pattern. By default, React components should be "controlled" - exposing a callback and expecting their parent to control them. If a component needs to ability to also manage its own state (be an "uncontrolled" component) in particular situations the parent should still be able to take over and make it controlled simply by passing in a value for the prop. For instance, an `onModalClose` callback could change `isModalOpen` to `false` when it is ready to close the modal. For more detail and examples of this pattern, visit [DIMOC: Do It Myself or Callback](https://gist.github.com/jamesgpearce/53a6fc57677870f93248). |
| 33 | +- <a name="event-callbacks" href="#event-callbacks">#</a> Event callbacks should pass in the synthetic event, then a data object with contents that relate to the event. |
| 34 | +- <a name="boolean-prop-prefix" href="#boolean-prop-prefix">#</a> If a prop is a boolean, please prefix with `is` or `can` or suffix it with `-able`. Never default a prop to `true`. |
| 35 | +- <a name="use-checkprops" href="#use-checkprops">#</a> Add as many prop checking tests that will _only run in development_ as needed via `checkProp`. If the test can become an independent module and work in multiple components, add it to the `utilities` folder. |
| 36 | +- <a name="all-text-can-be-internationalized" href="#all-text-can-be-internationalized">#</a> Any text the user can read (including text for screenreaders) should be able to be set via a prop for internationalization. |
| 37 | +- <a name="different-react-component-hierarchy" href="#different-react-component-hierarchy">#</a> React component hierarchy doesn't always mean HTML tag hierarchy. Sometimes children become the wrapping component. |
| 38 | +- <a name="classnames" href="#classnames">#</a> This library makes extensive use of the [classnames](https://github.com/JedWatson/classnames) library for feeding conditional CSS classes into `className` attributes and allows a variety of types such as `string`, `object`, and `arrays`. Please review the libary's API. |
| 39 | +- <a name="props-in-get-initial-state" href="#props-in-get-initial-state">#</a> [Props in getInitialState is an anti-pattern.](https://facebook.github.io/react/tips/props-in-getInitialState-as-anti-pattern.html) |
| 40 | +- <a name="jsx-gotchas" href="#jsx-gotchas">#</a> Read [JSX Gotchas](https://facebook.github.io/react/docs/jsx-gotchas.html#html-entities) |
| 41 | + |
| 42 | +## Component Organization |
| 43 | + |
| 44 | +* `createClass` |
| 45 | + * display name |
| 46 | + * prop types |
| 47 | + * defaults and initial state |
| 48 | + * life cycle methods |
| 49 | + * sub-render methods |
| 50 | + * primary render |
| 51 | + |
| 52 | +```javascript |
| 53 | +import { EXTERNAL_CONSTANT } from '../../utilities/constants'; |
| 54 | + |
| 55 | +/** |
| 56 | + * The description of this component (will appear in the documentation site). |
| 57 | + */ |
| 58 | +const DemoComponent = React.createClass({ |
| 59 | + displayName: EXTERNAL_CONSTANT, |
| 60 | + propTypes: { |
| 61 | + /** |
| 62 | + * The description of this prop (will appear in the documentation site). |
| 63 | + */ |
| 64 | + title: React.PropTypes.string.isRequired |
| 65 | + }, |
| 66 | + |
| 67 | + // These values will also be visible in the documentation site. |
| 68 | + getDefaultProps () { |
| 69 | + return { |
| 70 | + }; |
| 71 | + }, |
| 72 | + |
| 73 | + getInitialState () { |
| 74 | + return { |
| 75 | + }; |
| 76 | + }, |
| 77 | + |
| 78 | + toggleOpen (event) { |
| 79 | + }, |
| 80 | + |
| 81 | + renderSubComponent () { |
| 82 | + }, |
| 83 | + |
| 84 | + // Render should be last |
| 85 | + render () { |
| 86 | + } |
| 87 | +}); |
| 88 | + |
| 89 | +export default DemoComponent; |
| 90 | +``` |
| 91 | + |
| 92 | +## Formatting Props |
| 93 | + |
| 94 | +Wrap props on newlines for exactly 2 or more. Always list alphabetically. |
| 95 | + |
| 96 | +```html |
| 97 | +// bad |
| 98 | +<Person |
| 99 | + firstName="Michael" /> |
| 100 | + |
| 101 | +// good |
| 102 | +<Person firstName="Michael" /> |
| 103 | +``` |
| 104 | + |
| 105 | +```html |
| 106 | +// bad |
| 107 | +<Person firstName="Michael" lastName="Chan" occupation="Designer" favoriteFood="Drunken Noodles" /> |
| 108 | + |
| 109 | +// good |
| 110 | +<Person |
| 111 | + favoriteFood="Drunken Noodles" |
| 112 | + firstName="Michael" |
| 113 | + lastName="Chan" |
| 114 | + occupation="Designer" |
| 115 | +/> |
| 116 | +``` |
| 117 | + |
| 118 | +## Understand child component decorator pattern |
| 119 | +Limit aliasing of props for child components that already exist. This pattern is similar to [higher-order components](https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-components-94a0d2f9e750#.gjdiuf68s). It creates a separation of concern and a more declarative approach that relies on child components with their own props instead of additional props on the parent component. |
| 120 | + |
| 121 | +For instance, allowing `MenuDropdown` to have a `Trigger` child that can be a `Button` can prevent the creation of new props such as `buttonClassName` on `MenuDropdown`, since `Button` already has a `className` prop. |
| 122 | + |
| 123 | +- This reduces `prop` duplication for `props` that will just be passed down to the child component. |
| 124 | +- It allows `MenuDropdown` to decorate `Button` with the correct `onClick` among other `props`. |
| 125 | +- It allows the end-developer to use all existing `Button` props that they may already be familiar with. |
| 126 | + |
| 127 | +The following is a simple example of the cloning process within the parent. |
| 128 | + |
| 129 | + |
| 130 | +```javascript |
| 131 | +const CleverParent = React.createClass({ |
| 132 | + render() { |
| 133 | + const children = React.Children.map(this.props.children, (child) => { |
| 134 | + return React.cloneElement(child, { |
| 135 | + onClick: () => alert(JSON.stringify(child.props, 0, 2)) |
| 136 | + }) |
| 137 | + }) |
| 138 | + return <div>{children}</div> |
| 139 | + } |
| 140 | +}) |
| 141 | + |
| 142 | +const SimpleChild = React.createClass({ |
| 143 | + render() { |
| 144 | + return ( |
| 145 | + <div onClick={this.props.onClick}> |
| 146 | + {this.props.children} |
| 147 | + </div> |
| 148 | + ) |
| 149 | + } |
| 150 | +}) |
| 151 | + |
| 152 | +const App = React.createClass({ |
| 153 | + render() { |
| 154 | + return ( |
| 155 | + <CleverParent> |
| 156 | + <SimpleChild>1</SimpleChild> |
| 157 | + <SimpleChild>2</SimpleChild> |
| 158 | + <SimpleChild>3</SimpleChild> |
| 159 | + <SimpleChild>4</SimpleChild> |
| 160 | + <SimpleChild>5</SimpleChild> |
| 161 | + </CleverParent> |
| 162 | + ) |
| 163 | + } |
| 164 | +}) |
| 165 | +``` |
| 166 | +Example taken from [React Composability Patterns](http://www.zhubert.com/blog/2016/02/05/react-composability-patterns/) |
| 167 | + |
| 168 | + |
| 169 | +## Prefer Ternary to Sub-render |
| 170 | + |
| 171 | +Keep login inside the `render`. |
| 172 | + |
| 173 | +```javascript |
| 174 | +// bad |
| 175 | +renderSmilingStatement () { |
| 176 | + return <div>{this.props.name}{(this.state.isSmiling) ? <span> is smiling</span> : ""}</div>; |
| 177 | +}, |
| 178 | + |
| 179 | +render () { |
| 180 | + return <div>{this.props.name}{this.renderSmilingStatement()}</div>; |
| 181 | +} |
| 182 | +``` |
| 183 | + |
| 184 | +```javascript |
| 185 | +// good |
| 186 | +render () { |
| 187 | + return ( |
| 188 | + <div> |
| 189 | + {this.props.name} |
| 190 | + {this.state.isSmiling |
| 191 | + ? <span> is smiling</span> |
| 192 | + : null |
| 193 | + } |
| 194 | + </div> |
| 195 | + ); |
| 196 | +} |
| 197 | +``` |
| 198 | + |
| 199 | +## Naming Handler Methods |
| 200 | + |
| 201 | +Name the handler methods after their triggering event. |
| 202 | + |
| 203 | +```javascript |
| 204 | +// bad |
| 205 | +punchABadger () { /*...*/ }, |
| 206 | + |
| 207 | +render () { |
| 208 | + return <div onClick={this.punchABadger} />; |
| 209 | +} |
| 210 | +``` |
| 211 | + |
| 212 | +```javascript |
| 213 | +// good |
| 214 | +handleClick () { /*...*/ }, |
| 215 | + |
| 216 | +render () { |
| 217 | + return <div onClick={this.handleClick} />; |
| 218 | +} |
| 219 | +``` |
| 220 | + |
| 221 | +Handler names should: |
| 222 | + |
| 223 | +- begin with `handle` |
| 224 | +- end with the name of the event they handle (eg, `Click`, `Change`) |
| 225 | +- be present-tense |
| 226 | + |
| 227 | +If you need to disambiguate handlers, add additional information between |
| 228 | +`handle` and the event name. For example, you can distinguish between `onChange` |
| 229 | +handlers: `handleNameChange` and `handleAgeChange`. When you do this, ask |
| 230 | +yourself if you should be creating a new component. |
| 231 | + |
| 232 | +## Classnames |
| 233 | + |
| 234 | +Use [classNames](https://www.npmjs.com/package/classnames) to manage conditional classes. |
| 235 | + |
| 236 | +```javascript |
| 237 | +// bad |
| 238 | +get classes () { |
| 239 | + let classes = ['MyComponent']; |
| 240 | + |
| 241 | + if (this.state.active) { |
| 242 | + classes.push('MyComponent--active'); |
| 243 | + } |
| 244 | + |
| 245 | + return classes.join(' '); |
| 246 | +} |
| 247 | + |
| 248 | +render () { |
| 249 | + return <div className={this.classes} />; |
| 250 | +} |
| 251 | +``` |
| 252 | + |
| 253 | +```javascript |
| 254 | +// good |
| 255 | +render () { |
| 256 | + let classes = { |
| 257 | + 'MyComponent': true, |
| 258 | + 'MyComponent--active': this.state.active |
| 259 | + }; |
| 260 | + |
| 261 | + return <div className={classnames(classes)} />; |
| 262 | +} |
| 263 | +``` |
| 264 | + |
| 265 | +Read: [Class Name Manipulation](https://github.com/JedWatson/classnames/blob/master/README.md) |
| 266 | + |
| 267 | +from the [Planning Center](https://github.com/planningcenter/react-patterns) |
| 268 | + |
| 269 | +## Testing Guidelines |
| 270 | + |
| 271 | +- All external APIs should be tested, so that breaking changes can be detected. If a breaking change doesn't cause at least one test to fail, then add a test. |
| 272 | + - All `props` should be tested. It is OK to test multiple props in the same test for optmization as long as they are isolated and do not affect each other (for instance `id`, `classname`, and `style`). |
| 273 | + - All event callbacks should be tested along with any data object keys outside of the synthetic event to confirm the data. The data object, if present, is typically the second parameter of an event callback. |
| 274 | + - All mouse and keyboard interactions should be tested. |
| 275 | +- Components should have 90%+ test coverage. Coverage can be determined by reviewing the coverage summary at the end of `npm test`. Please note that high test coverage does not imply correct logic, but low coverage implies low test quality/quantity. |
| 276 | +- Test should run correctly in headless browsers (`npm test`) and within a "real" browser (`npm start` -> `http://localhost:8001/`) |
| 277 | +- For more specifics about testing please review the [testing module walkthough](tests/README.md). |
| 278 | + |
| 279 | + |
| 280 | +## Finalize new component/features |
| 281 | + |
| 282 | +1. Write tests for your new component/feature. |
| 283 | +2. Run `npm test`. |
| 284 | +3. After your PR is merged, make sure it appears here: [https://design-system-react-components.herokuapp.com](https://preview:8f2924b3d2232a37f63c32f70d9b3aba@design-system-react-components.herokuapp.com/). If it doesn't, reach out to one of the following people: |
| 285 | + * [Donielle Berg](https://github.com/donnieberg) |
| 286 | + * [Ivan Bogdanov](https://github.com/madpotato) |
| 287 | + * [David Brainer](https://github.com/tweettypography) |
| 288 | + * [Stephen James](https://github.com/interactivellama) |
| 289 | + * [David Woodward](https://github.com/futuremint) |
| 290 | +4. Get your component/feature approved by the UX Accessibility Team (refer to the link above). |
| 291 | + |
| 292 | + |
| 293 | +# Releasing |
| 294 | +1. [Add to release notes](https://github.com/salesforce-ux/design-system-react/blob/master/RELEASENOTES.md). |
| 295 | +1. Run `npm prune` and `npm install` to clean up node modules in preparation for build. |
| 296 | +1. Pull from upstream, bump and commit the package version, and publish tags to your upstream repo (that is this repo). **Choose one**: `npm run release-patch` or `npm run release-minor` |
| 297 | +1. Copy and paste your release notes into the Github Draft Release UI and publish. |
| 298 | + |
| 299 | +_If you are timid about releasing or need your pull request in review "pre-released," you can publish to origin (your fork) with `npm run publish-to-git` and then test and review the tag on your fork. This is just the publish step though, any other tasks you will need to do manually to test publishing._ |
0 commit comments