I was working on a refactor, I thought I would create this gist as a way to show some patterns that I like.
The rewritten file is ResourceHistoryMaster-AFTER.jsx. I think it's worth
looking over the original file, before looking at the new one.
The rewritten file is heavily commented.
| import React from "react"; | |
| import ResourceActions from "actions/ResourceActions"; | |
| import subscribe from "utilities/subscribe"; | |
| import RaisedButton from "material-ui/RaisedButton"; | |
| export default subscribe(React.createClass({ | |
| displayName: "MyResourceRequestsPage", | |
| // This handles all the resource logic | |
| fetch() { | |
| // By using props.subscriptions we're guaranteed that our app will | |
| // rerender when these stores are populated. | |
| let { | |
| IdentityStore, ProfileStore, StatusStore, ResourceRequestStore | |
| } = this.props.subscriptions; | |
| let profile = ProfileStore.get(); | |
| let statuses = StatusStore.getAll(); | |
| // I like to declare the variable above the statement where it is | |
| // assigned a value. Very quickly I can read this and know, that I'm | |
| // assigning requests, and request dependes on profile | |
| let requests; | |
| if (profile) { | |
| requests = ResourceRequestStore.findWhere({ | |
| "created_by.username": profile.get('username') | |
| }); | |
| } | |
| // Return an object with all resources, even if some are not resolved | |
| // yet, because: | |
| // 1. This makes the structure obvious | |
| // 2. This allows for destructuring that won't throw errors on missing | |
| // props in render() below | |
| return { | |
| statuses, | |
| requests, | |
| profile | |
| } | |
| }, | |
| // Although this function does end up with calling a closeAction, its | |
| // completely parametrized. Said differently, it's a function of its | |
| // inputs. This is much easier to test than if statuses, was moved inside | |
| // the function as a store.getAll call. | |
| onClose(request, statuses) { | |
| let closedStatus = statuses.findWhere({ "name": "closed" }); | |
| ResourceActions.close({ | |
| request: request, | |
| status: closedStatus | |
| }); | |
| }, | |
| render() { | |
| let { statuses, requests, profile } = this.fetch(); | |
| // Render here is completely unconcerned with the logic of fetching | |
| // resources, it just knows that if they are falsy, it should "load" | |
| if (!(requests && statuses && profile)) { | |
| return <div className="loading" /> | |
| } | |
| let viewProps = { | |
| statuses, | |
| requests, | |
| onClose: this.onClose | |
| }; | |
| // Here we call a separate component to do the dumb view stuff. It's | |
| // the easiest component to test, its just a function of its props. | |
| // Because this parent component did all the loading, the view doesn't | |
| // have to worry if its resources are present, which is really nice, | |
| // no wrapping if (resource) concerns. | |
| return <MyResourceRequestsPageView {...viewProps} /> | |
| } | |
| }), ["ProfileStore", "StatusStore", "ResourceRequestStore"]); | |
| let MyResourceRequestsPageView = React.createClass({ | |
| displayName: "MyResourceRequestsPageView", | |
| propTypes: { | |
| statuses: React.PropTypes.object.isRequired, | |
| requests: React.PropTypes.object.isRequired, | |
| onClose: React.PropTypes.func.isRequired | |
| }, | |
| renderRequestRow(request) { | |
| let statuses = this.props.statuses; | |
| let statusName = request.get("status").name; | |
| let className = "active" | |
| if (statusName == "approved") { | |
| className = "success"; | |
| } else if (statusName == "denied") { | |
| className = "denied"; | |
| } | |
| let closeButton = ( | |
| <RaisedButton primary | |
| className="pull-right" | |
| onTouchTap={() => this.props.onClose(request, statuses)} | |
| label="Close" /> | |
| ); | |
| // I want to draw attention to the line below with the ternary. There | |
| // are other ways to do this. I could have give the closeButton a | |
| // default of null above, and gotten rid of the ternary below. It's | |
| // really easy to make jsx /much/ less readable as you start throwing | |
| // expressions into it. | |
| // Compare: | |
| // { closeButton } | |
| // { statusName == "pending" ? closeButton : null} | |
| // | |
| // So I normally advocate for moving complex expressions out of jsx. A | |
| // template language's greatest offering is its clarity (think about | |
| // writing html in js). However I prefer the ternary because it's not | |
| // deceiving. When you read { closeButton } you would likely assume | |
| // that each table row has a button. | |
| return ( | |
| <tr key={request.id} className={className}> | |
| <td className="col-md-5"> | |
| {request.get("request")} | |
| </td> | |
| <td className="col-md-5"> | |
| {request.get("description")} | |
| </td> | |
| <td className="col-md-2"> | |
| {statusName} | |
| {statusName == "pending" ? closeButton : null} | |
| </td> | |
| </tr> | |
| ); | |
| }, | |
| renderBody() { | |
| let requests = this.props.requests; | |
| // This is just a short example of writing code to minimize | |
| // indentation. Since I'm returning here, I can use the space below | |
| // the if as an implicit else. I always move the simple returns to the | |
| // top, and leave the complex highly indented bit below. | |
| if (!requests.length) { | |
| return ( | |
| <p>You have not made any resource requests.</p> | |
| ); | |
| } | |
| return ( | |
| <table className="table table-condensed image-requests"> | |
| <tbody> | |
| <tr> | |
| <th> | |
| <h3 className="t-title">Request</h3> | |
| </th> | |
| <th> | |
| <h3 className="t-title">Reason</h3> | |
| </th> | |
| <th> | |
| <h3 className="t-title">Status</h3> | |
| </th> | |
| </tr> | |
| { requests.map(this.renderRequestRow) } | |
| </tbody> | |
| </table> | |
| ); | |
| }, | |
| render() { | |
| return ( | |
| <div className="container"> | |
| { this.renderBody() } | |
| </div> | |
| ); | |
| } | |
| }); |
| import React from "react"; | |
| import ResourceActions from "actions/ResourceActions"; | |
| import subscribe from "utilities/subscribe"; | |
| import RaisedButton from "material-ui/RaisedButton"; | |
| const MyResourceRequestsPage = React.createClass({ | |
| displayName: "MyResourceRequestsPage", | |
| closeRequest: function(request) { | |
| let { StatusStore } = this.props.subscriptions; | |
| var closedStatus = StatusStore.findWhere({ | |
| "name": "closed" | |
| }).models[0].id; | |
| ResourceActions.close({ | |
| request: request, | |
| status: closedStatus | |
| }); | |
| }, | |
| render: function() { | |
| let { IdentityStore, ProfileStore, StatusStore, ResourceRequestStore } = this.props.subscriptions; | |
| var username = ProfileStore.get().id, | |
| statusTypes = StatusStore.getAll(); | |
| if (username == null || !statusTypes) { | |
| return <div className="loading"></div> | |
| } | |
| var requests = ResourceRequestStore.findWhere({ | |
| "created_by.username": username | |
| }); | |
| if (requests == null) { | |
| return <div className="loading"></div>; | |
| } | |
| if (!requests.models[0]) { | |
| return ( | |
| <div className="container"> | |
| <p> | |
| You have not made any resource requests. | |
| </p> | |
| </div> | |
| ); | |
| } | |
| var displayRequests = requests.map(function(request) { | |
| // Handler to allow close buton to call React class closeRequest with the proper argument | |
| var close = function() { | |
| this.closeRequest(request) | |
| }.bind(this); | |
| var closeButton; | |
| var text = request.get("status").name; | |
| // set the color of the row based on the status of the request | |
| var trClass = "active"; | |
| if (text === "approved") { | |
| trClass = "success"; | |
| } else if (text === "denied") { | |
| trClass = "denied"; | |
| } else if (text === "pending") { | |
| closeButton = ( | |
| <RaisedButton | |
| primary | |
| className="pull-right" | |
| onTouchTap={close} | |
| label="Close" | |
| /> | |
| ); | |
| } | |
| return ( | |
| <tr key={request.id} className={trClass}> | |
| <td className="col-md-5"> | |
| {request.get("request")} | |
| </td> | |
| <td className="col-md-5"> | |
| {request.get("description")} | |
| </td> | |
| <td className="col-md-2"> | |
| {request.get("status").name} | |
| {closeButton} | |
| </td> | |
| </tr> | |
| ); | |
| }.bind(this)); | |
| return ( | |
| <div className="container"> | |
| <table className="table table-condensed image-requests"> | |
| <tbody> | |
| <tr> | |
| <th> | |
| <h3 className="t-title">Request</h3> | |
| </th> | |
| <th> | |
| <h3 className="t-title">Reason</h3> | |
| </th> | |
| <th> | |
| <h3 className="t-title">Status</h3> | |
| </th> | |
| </tr> | |
| {displayRequests} | |
| </tbody> | |
| </table> | |
| </div> | |
| ); | |
| } | |
| }); | |
| export default subscribe(MyResourceRequestsPage, ["ProfileStore", "StatusStore", "ResourceRequestStore"]); |
Stay tuned! :)