@ReactMeteor Someone kindly had a look through our code and suggested some improvements. Please review their suggestions and implement them, or create tasks here for them, if you agree (as two people are in favour), or comment here if you disagree with them so others can join in and break the tie
- A lot of use of global variables stored in window object, which reduces reusability. For example window.__setDocumentTitle can simply be a utility function .
- The code contains a few deprecated functions, but it’s understandable, since it’s not the newest. (for example string.substr)
- Class components are used thoughout the app making the code verbose in some places. Also think of only passing necessary props to children. For example:
// before
class Events extends Component {
constructor(props) {
super(props);
this.state = { }
}
render() {
const { userEvents, user, isAllEvents, deleteAllEvents } = this.props;
const events = userEvents.filter(ele => {
return ele.organiser._id === user._id;
})
return (
<React.Fragment>
< EventsDisplay userEvents={events} isAllEvents={isAllEvents} deleteAllEvents={deleteAllEvents} />
</React.Fragment>
)
}
}
// after
const OrganizerEvents = ({ userEvents, orgernazierId, ...props }) => (
<EventsDisplay
userEvents={userEvents.filter(
({ organiser }) => organiser._id === orgernazierId
)}
{...props}
/>
);
- You can also deconstruct props to make the code cleaner
// before
const idToDelete = this.props.idToDelete;
const deleteDocument = this.props.deleteDocument;
const deleteText = this.props.deleteText;
// after
const { idToDelete, deleteDocument, deleteText } = this.props;
- In CancelDeleteBtn, prefer creating components globally and not inside another ones.
- in CategoryFropDown.render you create an array and push components to it. I would prefer to simply map the array to elements instead, since it’s a common pattern.
// before
render() {
const { allPosibleCategories} = this.props;
const categoriesForDropDown = [<option key={"Sort By Category"} disabled>Sort By Category</option>];
allPosibleCategories.forEach(ele=>{
categoriesForDropDown.push(<option key={ele} value={ele}>{ele}</option>);
})
return (
<select id="category-select" onChange={this.change}>
{categoriesForDropDown}
</select>
)
}
// after
render = () => (
<select id="category-select" onChange={this.change}>
<option key={"Sort By Category"} disabled>
Sort By Category
</option>
{this.props.allPosibleCategories.map((ele) => (
<option key={ele} value={ele}>
{ele}
</option>
))}
</select>
);
- Also instead of using componentDidUpdate, prefer using controlled components with useState (checkout <input> – React)