|
| 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 | +- This library should include only components which have approved patterns in SLDS. |
| 21 | +- Familiarize yourself with concepts used in the rest of the library. |
| 22 | +- 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 | +- `React.createClass` is preferred over ES6 classes and `extend` at this time. |
| 24 | +- Prefer stateless components (basically a one fat-arrow functional component) unless you need state. Try not to use state. |
| 25 | +- 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. |
| 26 | +- 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." In short, don't utilize any properties in the `...props` object within the component. After using `const { active, className, ...other } = props;` do not go back to using this.prop.* anywhere in the render function. |
| 27 | +- 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). |
| 28 | +- Event callbacks should pass in the synthetic event, then a data object with contents that relate to the event. |
| 29 | +- If a prop is a boolean, please prefix with `is` or `can` or suffix it with `-able`. Never default a prop to `true`. |
| 30 | +- 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. |
| 31 | +- Any text the user can read (including a11y text for screenreaders) should be able to be set via a prop for internationalization. |
| 32 | +- React component hierarchy doesn't always mean HTML tag hierarchy. Sometimes children become the wrapping component. |
| 33 | +- [Props in getInitialState is an anti-pattern.](https://facebook.github.io/react/tips/props-in-getInitialState-as-anti-pattern.html) |
| 34 | +- Read [JSX Gotchas](https://facebook.github.io/react/docs/jsx-gotchas.html#html-entities) |
| 35 | + |
| 36 | +## Component Organization |
| 37 | + |
| 38 | +* `createClass` |
| 39 | + * display name |
| 40 | + * prop types |
| 41 | + * defaults and initial state |
| 42 | + * life cycle methods |
| 43 | + * sub-render methods |
| 44 | + * primary render |
| 45 | + |
| 46 | +```javascript |
| 47 | +import { EXTERNAL_CONSTANT } from '../../utilities/constants'; |
| 48 | + |
| 49 | +/** |
| 50 | + * The description of this component (will appear in the documentation site). |
| 51 | + */ |
| 52 | +const DemoComponent = React.createClass({ |
| 53 | + displayName: EXTERNAL_CONSTANT, |
| 54 | + propTypes: { |
| 55 | + /** |
| 56 | + * The description of this prop (will appear in the documentation site). |
| 57 | + */ |
| 58 | + title: React.PropTypes.string.isRequired |
| 59 | + }, |
| 60 | + |
| 61 | + // These values will also be visible in the documentation site. |
| 62 | + getDefaultProps () { |
| 63 | + return { |
| 64 | + }; |
| 65 | + }, |
| 66 | + |
| 67 | + getInitialState () { |
| 68 | + return { |
| 69 | + }; |
| 70 | + }, |
| 71 | + |
| 72 | + toggleOpen (event) { |
| 73 | + }, |
| 74 | + |
| 75 | + renderSubComponent () { |
| 76 | + }, |
| 77 | + |
| 78 | + // Render should be last |
| 79 | + render () { |
| 80 | + } |
| 81 | +}); |
| 82 | + |
| 83 | +export default DemoComponent; |
| 84 | +``` |
| 85 | + |
| 86 | +## Formatting Props |
| 87 | + |
| 88 | +Wrap props on newlines for exactly 2 or more. Always list alphabetically. |
| 89 | + |
| 90 | +```html |
| 91 | +// bad |
| 92 | +<Person |
| 93 | + firstName="Michael" /> |
| 94 | + |
| 95 | +// good |
| 96 | +<Person firstName="Michael" /> |
| 97 | +``` |
| 98 | + |
| 99 | +```html |
| 100 | +// bad |
| 101 | +<Person firstName="Michael" lastName="Chan" occupation="Designer" favoriteFood="Drunken Noodles" /> |
| 102 | + |
| 103 | +// good |
| 104 | +<Person |
| 105 | + favoriteFood="Drunken Noodles" |
| 106 | + firstName="Michael" |
| 107 | + lastName="Chan" |
| 108 | + occupation="Designer" |
| 109 | +/> |
| 110 | +``` |
| 111 | + |
| 112 | +## Understand child component decorator pattern |
| 113 | +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. |
| 114 | + |
| 115 | +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. |
| 116 | + |
| 117 | +- This reduces `prop` duplication for `props` that will just be passed down to the child component. |
| 118 | +- It allows `MenuDropdown` to decorate `Button` with the correct `onClick` among other `props`. |
| 119 | +- It allows the end-developer to use all existing `Button` props that they may already be familiar with. |
| 120 | + |
| 121 | +The following is a simple example of the cloning process within the parent. |
| 122 | + |
| 123 | + |
| 124 | +```javascript |
| 125 | +const CleverParent = React.createClass({ |
| 126 | + render() { |
| 127 | + const children = React.Children.map(this.props.children, (child) => { |
| 128 | + return React.cloneElement(child, { |
| 129 | + onClick: () => alert(JSON.stringify(child.props, 0, 2)) |
| 130 | + }) |
| 131 | + }) |
| 132 | + return <div>{children}</div> |
| 133 | + } |
| 134 | +}) |
| 135 | + |
| 136 | +const SimpleChild = React.createClass({ |
| 137 | + render() { |
| 138 | + return ( |
| 139 | + <div onClick={this.props.onClick}> |
| 140 | + {this.props.children} |
| 141 | + </div> |
| 142 | + ) |
| 143 | + } |
| 144 | +}) |
| 145 | + |
| 146 | +const App = React.createClass({ |
| 147 | + render() { |
| 148 | + return ( |
| 149 | + <CleverParent> |
| 150 | + <SimpleChild>1</SimpleChild> |
| 151 | + <SimpleChild>2</SimpleChild> |
| 152 | + <SimpleChild>3</SimpleChild> |
| 153 | + <SimpleChild>4</SimpleChild> |
| 154 | + <SimpleChild>5</SimpleChild> |
| 155 | + </CleverParent> |
| 156 | + ) |
| 157 | + } |
| 158 | +}) |
| 159 | +``` |
| 160 | +Example taken from [React Composability Patterns](http://www.zhubert.com/blog/2016/02/05/react-composability-patterns/) |
| 161 | + |
| 162 | + |
| 163 | +## Prefer Ternary to Sub-render |
| 164 | + |
| 165 | +Keep login inside the `render`. |
| 166 | + |
| 167 | +```javascript |
| 168 | +// bad |
| 169 | +renderSmilingStatement () { |
| 170 | + return <div>{this.props.name}{(this.state.isSmiling) ? <span> is smiling</span> : ""}</div>; |
| 171 | +}, |
| 172 | + |
| 173 | +render () { |
| 174 | + return <div>{this.props.name}{this.renderSmilingStatement()}</div>; |
| 175 | +} |
| 176 | +``` |
| 177 | + |
| 178 | +```javascript |
| 179 | +// good |
| 180 | +render () { |
| 181 | + return ( |
| 182 | + <div> |
| 183 | + {this.props.name} |
| 184 | + {this.state.isSmiling |
| 185 | + ? <span> is smiling</span> |
| 186 | + : null |
| 187 | + } |
| 188 | + </div> |
| 189 | + ); |
| 190 | +} |
| 191 | +``` |
| 192 | + |
| 193 | +## Naming Handler Methods |
| 194 | + |
| 195 | +Name the handler methods after their triggering event. |
| 196 | + |
| 197 | +```javascript |
| 198 | +// bad |
| 199 | +punchABadger () { /*...*/ }, |
| 200 | + |
| 201 | +render () { |
| 202 | + return <div onClick={this.punchABadger} />; |
| 203 | +} |
| 204 | +``` |
| 205 | + |
| 206 | +```javascript |
| 207 | +// good |
| 208 | +handleClick () { /*...*/ }, |
| 209 | + |
| 210 | +render () { |
| 211 | + return <div onClick={this.handleClick} />; |
| 212 | +} |
| 213 | +``` |
| 214 | + |
| 215 | +Handler names should: |
| 216 | + |
| 217 | +- begin with `handle` |
| 218 | +- end with the name of the event they handle (eg, `Click`, `Change`) |
| 219 | +- be present-tense |
| 220 | + |
| 221 | +If you need to disambiguate handlers, add additional information between |
| 222 | +`handle` and the event name. For example, you can distinguish between `onChange` |
| 223 | +handlers: `handleNameChange` and `handleAgeChange`. When you do this, ask |
| 224 | +yourself if you should be creating a new component. |
| 225 | + |
| 226 | +## Classnames |
| 227 | + |
| 228 | +Use [classNames](https://www.npmjs.com/package/classnames) to manage conditional classes. |
| 229 | + |
| 230 | +```javascript |
| 231 | +// bad |
| 232 | +get classes () { |
| 233 | + let classes = ['MyComponent']; |
| 234 | + |
| 235 | + if (this.state.active) { |
| 236 | + classes.push('MyComponent--active'); |
| 237 | + } |
| 238 | + |
| 239 | + return classes.join(' '); |
| 240 | +} |
| 241 | + |
| 242 | +render () { |
| 243 | + return <div className={this.classes} />; |
| 244 | +} |
| 245 | +``` |
| 246 | + |
| 247 | +```javascript |
| 248 | +// good |
| 249 | +render () { |
| 250 | + let classes = { |
| 251 | + 'MyComponent': true, |
| 252 | + 'MyComponent--active': this.state.active |
| 253 | + }; |
| 254 | + |
| 255 | + return <div className={classnames(classes)} />; |
| 256 | +} |
| 257 | +``` |
| 258 | + |
| 259 | +Read: [Class Name Manipulation](https://github.com/JedWatson/classnames/blob/master/README.md) |
| 260 | + |
| 261 | +from the [Planning Center](https://github.com/planningcenter/react-patterns) |
| 262 | + |
| 263 | +## Finalize new component/features |
| 264 | + |
| 265 | +1. Write tests for your new component/feature. |
| 266 | +2. Run `npm test`. |
| 267 | +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: |
| 268 | + * [Donielle Berg](https://github.com/donnieberg) |
| 269 | + * [Ivan Bogdanov](https://github.com/madpotato) |
| 270 | + * [David Brainer](https://github.com/tweettypography) |
| 271 | +4. Get your component/feature approved by the UX Accessibility Team (refer to the link above). |
| 272 | + |
| 273 | + |
| 274 | +# Releasing |
| 275 | +1. Run `npm prune` and `npm install` to clean up node modules in preparation for build. |
| 276 | +2. Increment the package version in `package.json` based on the `semver` methodology. |
| 277 | +3. [Add to release notes](https://github.com/salesforce-ux/design-system-react/blob/master/RELEASENOTES.md) |
| 278 | +4. Commit the previous two changes. |
| 279 | +5. Publish to your upstream repo (that is this repo): `npm run publish-to-upstream` |
| 280 | +6. Copy and paste your release notes into the Github Draft Release UI and publish. |
| 281 | + |
| 282 | +_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._ |
0 commit comments