diff --git a/react/README.md b/react/README.md
index 6d69b5e..dde0885 100644
--- a/react/README.md
+++ b/react/README.md
@@ -1621,49 +1621,189 @@ In addition to `render()` being the _last_ method in a component (see [Method or
### Logic and JSX
-React and JSX supporting logic and markup in the same file allows for substantial complexity in markup generation over other traditional templating languages (like [handlebars](http://handlebarsjs.com)). But with that increased complexity can come a decrease in readability.
+> **Note:** This rule was changed from a previous version that said the
+> opposite, read the [refactoring](../refactors/logic-and-jsx) that explains why.
-In order to maximize both complexity and readability, we suggest keeping all logic out of JSX, except variable references and method calls. Expressions, particularly ternary expressions, should be stored in variables outside of JSX.
+A single component's returned JSX code can get quite complex with lots of
+expressions. But instead of pulling those expressions out into variables and
+statements, you should focus on pulling out parts of the JSX into seperate
+components, and some of the logic into utility functions.
```js
-// good
-render() {
- let {includeHeader} = this.props;
- let buttons = [1, 2, 3, 4, 5].map((page) => (
-
- ));
- let header;
+// good (inlining all JSX/simple logic and abstracting complex JSX into separate components)
+function Component1(props) {
+ return ;
+}
+
+function Component2(props) {
+ return ;
+}
+
+class MyComponent extends React.Component {
+ render() {
+ return (
+
);
+// bad (hard to refactor into seperate components / moves JSX around)
+class MyComponent extends React.Component {
+ render() {
+ let { foo, bar } = this.props;
+ let { baz } = this.state;
+
+ let element1 = null;
+ let element2 = null;
+
+ if (cond) {
+ if (otherCond) {
+ element1 = ;
+ } else {
+ element2 = ;
+ }
+ }
+
+ return (
+
+ {element1}
+ {element2}
+
+ );
}
+}
+```
+
+This might seem unnatural at first if you aren't familiar with writing React
+code this way. But if you start splitting JSX into logical statements, you end
+up with code that is hard to pull out into separate components, making your code
+less refactorable (and maintainable as a result).
+
+See [Helper components](#helper-components) for another way to help keep `render()` lean.
+
+#### Logic and JSX Tip #1: Complex Conditionals
+
+Sometimes inline JSX conditionals will get really complex. This can be hard to
+describe which logic is complex and which is not. But if it starts to become
+hard to read at a glance, you might want to pull that logic out into a small
+utility function.
+```js
+// bad (complex inline conditions)
+function MyComponent(props) {
return (
+ Hello, I also have a complex conditional render
+
+ )}
);
}
-// bad (expressions in JSX)
-render() {
- let {includeHeader} = this.props;
+// good (small utils for the conditional logic that we pass info into)
+function xorEq(a, b, value) {
+ if (a === value && b !== value) return true;
+ if (a !== value && b === value) return true;
+ return false;
+}
+
+function isPlainObject(val) {
+ return typeof val === 'object' && val !== null && !Array.isArray(val);
+}
+
+function isEmptyObject(obj) {
+ return Object.keys(obj).length === 0
+}
+function MyComponent(props) {
return (
);
}
```
-The above "bad" example doesn't seem so bad right? But as we know, code tends to grow over time. If we add more expressions, add more markup to the header, or the map gets more more logic, the code will become unwieldy. Setting up this guideline, even in the most simple examples, helps set the code along the right path.
+#### Logic and JSX Tip #2: Computed Data
-See [Helper components](#helper-components) for another way to help keep `render()` lean.
+Sometimes when you're writing JSX you'll start to compute data inline using
+functions like `array.map/reduce/filter/etc`. However, it's better to pull that
+logic out into separate functions, and store the data in variables before the
+JSX code.
+
+```js
+// bad (complex computed data written inline)
+function MyComponent(props) {
+ return (
+
+ );
+ };
+}
diff --git a/refactors/logic-and-jsx/README.md b/refactors/logic-and-jsx/README.md
new file mode 100644
index 0000000..d1d985e
--- /dev/null
+++ b/refactors/logic-and-jsx/README.md
@@ -0,0 +1,663 @@
+# Logic and JSX Refactor
+
+## Background
+
+The code in [`OriginalFile.js`](0-OriginalFile.js) comes directly out of one of
+our React apps in our Core repo. This file was becoming difficult to maintain,
+and so we looked into refactoring it.
+
+At the time this code was written, we had a React style guide rule about JSX
+that says the following:
+
+> React and JSX supporting logic and markup in the same file allows for
+> substantial complexity in markup generation over other traditional templating
+> languages (like handlebars). But with that increased complexity can come a
+> decrease in readability.
+>
+> In order to maximize both complexity and readability, we suggest keeping all
+> logic out of JSX, except variable references and method calls. Expressions,
+> particularly ternary expressions, should be stored in variables outside of
+> JSX.
+>
+> ```js
+> // good
+> render() {
+> let {includeHeader} = this.props;
+> let buttons = [1, 2, 3, 4, 5].map((page) => (
+>
+> ));
+> let header;
+>
+> if (includeHeader) {
+> header = (
Pagination
);
+> }
+>
+> return (
+>
+> {header}
+> {buttons}
+>
+> );
+> }
+> ```
+>
+> ```js
+> // bad (expressions in JSX)
+> render() {
+> let {includeHeader} = this.props;
+>
+> return (
+>
+> );
+> }
+> ```
+>
+> The above "bad" example doesn't seem so bad right? But as we know, code tends
+> to grow over time. If we add more expressions, add more markup to the header,
+> or the map gets more more logic, the code will become unwieldy. Setting up
+> this guideline, even in the most simple examples, helps set the code along the
+> right path.
+
+However, if you look at the `render()` method in
+[`OriginalFile.js`](0-OriginalFile.js) you'll see that, despite following this
+rule, it has become a bit of a mess.
+
+Looking at it quickly, it's hard to tell what is rendered when and what data it
+depends on and where that data comes from.
+
+Let's refactor this component, ignoring the above rule, to see how we can
+improve this real application code.
+
+## Step 1: [`NoDestructuring.js`](1-NoDestructuring.js)
+
+So the first step in untangling this render method is actually to eliminate the
+large amount of destructuring that is happening on the `props`, `state`, and
+some of the imports.
+
+#### Before:
+
+```js
+import {
+ ACCOUNT_SETTINGS_LIST_COLUMNS as columns,
+ DELETION_ALLOWED,
+ DELETION_EVENT_ASSOCIATED,
+ DELETION_API_ERROR_ASSOCIATED,
+ UPDATE_PAYMENT_INSTRUMENT_MODAL,
+ FORM_FROM_INSTRUMENT_TYPE,
+} from '../constants';
+
+// ...
+
+const {
+ eventList = [],
+ hideEventsModal,
+ showEventsModal,
+ isInstrumentTypeFilterActive = false,
+ apiHandledErrors = null,
+ getEvents,
+ isEventsModalOpen = false,
+ paymentInstruments: {
+ userInstruments,
+ pagination: {
+ pageCount = PAYMENT_INSTRUMENT_PAGINATION.PAGE_SIZE,
+ pageNumber = PAYMENT_INSTRUMENT_PAGINATION.DEFAULT_PAGE,
+ } = {},
+ } = {},
+ isUpdateIntrumentModalOpen,
+ hideUpdateInstrumentModal,
+ isInProgressDialogOpen,
+ hideInProgressDialog,
+ canListPayoutUserInstruments,
+} = this.props;
+
+// ...
+
+;
+```
+
+#### After:
+
+```js
+import * as Constants from '../constants';
+
+// ...
+
+;
+```
+
+We do this so all of the values we are referencing also include where we are
+referencing them from. This makes it easy to move code around because we know
+exactly where everything is coming from. Otherwise, we would have to track down
+a bunch of variables to make sure we maintain the correct references.
+
+## Step 2: [`SplitConditionals.js`](2-SplitConditionals.js)
+
+In our original file we had a lot of long if-then statements, some of which were
+nested. Inside of them we assigned a lot of JSX to variables we rendered later
+on in the `render()` method.
+
+Instead of this, we're going to split up all the JSX that is conditionally
+assigned to variables into their own sections.
+
+#### Before:
+
+```js
+let emptyState = null;
+let bottomText = null;
+let pagination = null;
+let filterPrimaryText = null;
+let filter = null;
+let paymentInstrumentTable = null;
+let eventsModal = null;
+let errorDialog = null;
+let paymentInstrumentModalsAndDialogs = null;
+let filterValues = FinancialProfileConstants.INSTRUMENT_TYPES_VALUE_DISPLAY;
+
+// ...
+
+paymentInstrumentTable = (
+
+);
+if (isEmpty(paymentInstrumentsItems)) {
+ if (this.props.isInstrumentTypeFilterActive) {
+ filterPrimaryText = gettext('Remove filter');
+ } else {
+ paymentInstrumentTable = null;
+ }
+ emptyState = (
+
+ );
+}
+
+// --------------------------------------------------------------------
+```
+
+Now we have something much easier to move around and refactor in more meaningful
+ways. We know exactly what is being rendered, when it is being rendered, and
+what data it needs in order to render. All in nice independent chunks that can
+be cut-and-pasted whereever we want it to go.
+
+We might have repeated some of our conditional logic, and even made the code a
+bit longer. But we've given ourselves something more meaningful:
+Refactorability.
+
+> **Aside:** It's hard to capture what exactly makes some code refactorable and
+> other hard to refactor. But if you're having trouble refactoring some code,
+> consider taking a few intermediate steps to untangle the code and make the
+> refactor easy for yourself. It's far easier to do major refactors in
+> individual steps than to try doing it all at once.
+
+## Step 3: [`InlineLogic.js`](3-InlineLogic.js)
+
+The next step we're going to take is going to go directly against the original
+rule we started out with. We're going to move our conditionally rendered JSX
+code directly into the `render()` method's `return (...)` JSX.
+
+#### Before:
+
+```js
+let filter = (
+
+);
+
+let eventsModal = null;
+if (!isEmpty(paymentInstrumentsItems) && this.props.isEventsModalOpen) {
+ eventsModal = (
+
+ );
+}
+
+let errorDialog = null;
+if (
+ !isEmpty(paymentInstrumentsItems) &&
+ !isEmpty(this.props.apiHandledErrors)
+) {
+ errorDialog = (
+
+ );
+}
+
+// ...
+
+return (
+
+);
+```
+
+If you're not used to writing React code like this, it might seem objectionably
+worse code. And you could make a pretty good argument that it is worse. We're
+not quite done yet, but I would like to take a second to argue that this step in
+our refactor is actually an improvement.
+
+We still have the benefits we had from before. Looking at any one part of this
+large chunk of JSX, we can easily tell what is being rendered, when it is being
+rendered, and what data we need in order to render it (and where that data is
+coming from).
+
+But we now have an additional benefit: We can very easily see where things are
+rendered in relation to one another. If we look at the "Before" code in
+[`SplitConditionals.js`](2-SplitConditionals.js), we're creating a bunch of
+variables that are kind of "floating", as in we don't immediately see where they
+are used. And you'll notice that if you scroll down to the final returned JSX,
+many of those variables aren't being referenced because they are actually used
+inside of other chunks of JSX.
+
+By putting all of the JSX into the `return ()`'d JSX, we can see it clearly as a
+tree, which is going to make the next refactor easier.
+
+## Step 4: [`PullIntoComponents.js`](4-PullIntoComponents.js)
+
+Our final step in this refactor is going to be to take chunks of our
+`return ()`'d JSX and pull them out into their own components.
+
+#### Before:
+
+```js
+return (
+
+);
+```
+
+Now our component's `render()` method is _just_ the information we care about
+for the current component, it contains:
+
+- What is being rendered.
+- When it is being rendered.
+- What data it needs in order to render.
+- Where things render in relation to one another.
+- And finally (and this is important), **none of the information we don't care
+ about to distract us.**
+
+### Tip 1: Complex conditionals should be pulled into separate functions with data passed in
+
+You'll notice that we've kept the inline conditionals. This is because overall
+they are pretty simple conditionals, all of them are boolean values.
+
+If we had something more complex, we might choose to pull that complexity back
+out into "selectors" or utilities that we can call inline.
+
+```js
+// Before:
+function MyComponent(props) {
+ return (
+
+ );
+}
+```
+
+### Tip 2: Computed data should be pulled out into variables.
+
+Doing more complex computations that create data for your JSX to render is tough
+to do inline. So in those cases we'll choose to pull the data out into a
+variable, and possibly pull the computation logic into its own function.
+
+```js
+// Before:
+function MyComponent(props) {
+ return (
+
+ );
+}
+```
+
+> **Note:** Be sure not to pull any of the JSX into these helper functions. As a
+> general rule: If you're writing JSX in a separate function that isn't a
+> component (or inside a class component's `render()` method), you're probably
+> doing something "weird".
+
+## Aftermath
+
+After this refactoring was presented, we decided to change our original rule to
+be more relaxed and allow for this style of refactoring.