7

as of React 16 version you can use <React.Fragment> <dt>Term</dt> <dd>Definition</dd> </React.Fragment> React.Fragment will not render any DOM, but it'll act as a container. You can also use a short hand for React.Fragment as <> <> <dt>Term</dt> <dd>Definition</dd> &...


7

I think everything Sam Onela said is great. I'm gonna add a few more things. Don't modify your state directly In general, it's considered bad practice to directly modify your state object. According to the react docs: Never mutate this.state directly, as calling setState() afterwards may replace the mutation you made. Treat this.state as if it were ...


6

return { tasks:tasks, numCompleted:null } numCompleted isn't being used anywhere. Remove if from the data. Also, it's a "computed property", one you can compute off from tasks. I notice you compute it on render. Consider putting it in a function instead. getCompletedTaskCount: function(){ return this.state.tasks.filter(task => task.completed)....


6

Feedback Nice use of arrow functions, the fetch API (and associated promises), etc. The layout looks nice as well. Suggestions/Review Points Options could appear multiple times The code in getRandomCountry() does not account for the scenario where Math.floor(Math.random()*this.state.countries.length) yields the same value within 4 consecutive calls. Even ...


6

The screen design looks very nice. It is minimalistic, yet pleasing. It almost perfectly fits on the small screen of my phone (only sometimes, I have to scroll down to see all the answer buttons). The flags are nicely big to explore the fine graphical details. To appreciate the shape of the Nepal flag, there should be no border around it, and the background ...


5

Move variables out of render All the variables you are storing are sitting in the render method of the respective component. This means that the variables are constantly created and destroyed. It would be better to either: Set them as properties of the object you are passing into React.createClass() Write the values inline with the returned elements. ...


5

I'm not exactly sure of your environment (I'm going to assume you're using Node), and with this assumption, I want to address a few things starting with specific things, then more general: Specific You shouldn't be keeping inputText in the global store. This creates lots of store dispatches and updates which are completely unnecessary and costly. Instead, ...


5

There are two distinct parts in your test: Testing the rendering of AvgSpeed component. Testing the logic of getAverage() and extractSpeeds(). This separation between components and logic is good. I would suggest separating them even more by moving the getAverage() and extractSpeeds() functions to separate file, which you can then test completely ...


4

I'll start from what I perceive as "top" and work my way down. <div> <form action="/api/blogPosts" method="post"> <input onChange={this.handleChange} type="text" name="postbody"></input> <button type="button" onClick={this.handleClick}>Submit</button> </form> <List array={this.state.posts} /> <...


4

I see some Redux conventions misconceptions: The reducer purpose is to change the state, I see that you also copy state to localStorage. You should use actionCreators to save data in localStorage. In actionCreator, you save data and then - dispatch action to reducer. Any of the React components with Redux don't need to have state. You should pass all ...


4

One thing I noticed is that you're passing this.handleUpvoteClicked.bind(this) as a prop to your FlatButton components. The problem is that the call to Function.prototype.bind creates a new function instance for both click handlers every time your Post renders, meaning your FlatButton components see a different value (function reference) being passed in as a ...


4

I see no particularly compelling reason to make CommentBoxContainer a class instead of a Stateless Functional Component, like most of the rest of the items here. CommentContainer also could be a SFC. In CommentListContainer, I would set: this.state = { data: this.props.data || [], numComments: this.props.data && this.props.data.length || 0 } ...


4

Use a way of checking for the jest global that won't throw an error: if (typeof jest == 'undefined') { console.log('some browser-only output'); } You can also take advantage of the NODE_ENV environment variable: if (process.env.NODE_ENV == 'development') { console.log('browser'); } else if (process.env.NODE_ENV == 'test') { console.log('jest'); }...


4

Hope this helps someone else: jest --silent http://facebook.github.io/jest/docs/en/cli.html#silent


4

First off, well done on getting it to work - that's often the most important and anything beyond that comes with experience and following the docs and guidelines. First question: In my experience your intuition to use a prop instead of storing the sounds in state is the right one. It is essentially ok the way you have it in state, but the less state your ...


4

Use ES2017 object spread: const uploadedFiles = images.map((item, index) => { const props = { key: index, src: item.blobUrl, onClose: this.props.removeImage, }; const isSingleGif = images.length === 1 && /\.gif$/i.test(item.file.name); if (isSingleGif) { return (<ImageGif {...props} />); } const isLastOfVisible ...


4

I think there are many things you could improve, I will try to review your code and give some pointers. Instead of using React.Component, you should directly access Component: import React, { Component } from 'react'; class SearchList extends Component { In older examples, you could find it in the way you do it, but more recent example use this form. ...


4

typeof activeEvents === 'object' && activeEvents !== null can be true for both Array and Object : const array = []; const obj = {}; console.log(typeof array === 'object' && array !== null) console.log(typeof obj === 'object' && obj !== null) you can use Array.isArray const activeEvents = [{ id: '1', title: 'event ...


4

Use Props ! Using props to highly customize your component is good practice, but too many props isn't always good ! You need to ask yourself: Am I trying to overly handle every situation possible ? Is it ok if a second component handles different cases ? Are these props absolutely required for my component to work as intended ? If 1. and/or 2. is a yes ...


4

if you're just trying to decide between number--high number--medium or '' based on the condition variable, then just make a helper function that does that then do return <div className={"number " + getNumberClass(condition)}}>{number}</div>


3

I haven't quite understood what Babel is used for at all. Babel is used for transpilation - the process of converting unsupported/future syntax into one that is supported at the moment. For instance, your destructuring assignment: const { Router, Route, IndexRoute, Redirect, Link, IndexLink } = ReactRouter ...


3

These lines let firstName = this.refs.firstName.value.trim(); look like a code duplication to me. Could they be rewritten into something more elegant? Looks ok to me. These fields are too few to be generalized by some generalization logic. Now if the form does grow, you can just get a reference to the <form> and do a quick querySelectorAll for all ...


3

I can't offer a full review for you right now, but here are a couple little things: A couple simplifications In ToDoContainer.render, this: var numCompleted = this.state.tasks.filter(function(task){ return task.completed; }).length var numRemaining = this.state.tasks.filter(function(task){ return !task.completed; }).length is doing extra work. ...


3

Ok, looks like a neatly written React. Let's see what I can do. <div className="main-content col-sm-12"> <div className="left-sided-lg-top-otherwise col-lg-8 col-md-12 col-sm-12 col-xs-12"> {loading ? <Loading /> : <BigStory newsItems={newsItems[0]}/> ...


3

2 suggestions. There can be many terms to one definition. So it's not general-purpose. Naming - use "term" and "definition" instead of "definition" and "value" to be similar to html elements.


3

You haven't shown any of the child components, but here's how I would approach this: Instead of having each of your components be generated in a giant switch statement, I'd use an object that maps a component name to a component itself. Each of these components all have the same prop keys, they are just different components, right? * Further, your recap ...


3

Maybe it would be better to handle onMouseUp={this.onDragEnd}? Also instead of const componentDefaults you can use OnOff.defaultProps. And if you want to share this component for other people, I recommend to specify OnOff.propTypes, it makes the code more stable and resistant. At the whole seems good, works on desktop and mobile as well.


3

You can use the setUpFiles option to configure files to be run before jest run tests. More info here: https://facebook.github.io/jest/docs/configuration.html#setupfiles-array Then in your set up file, you can disable logging. For example, to disable console.log, you can do console.log = function() {}


3

Choosing the heading level at runtime: According to the official docs, you cannot use expressions within JSX types. However, you can use capitalized variables: const Heading = "h" + 1; return <Heading>...</Heading>; Further suggestions: Rename TypographyHeader to TypographyHeading to avoid confusion with <header>. Rename headerType to ...


3

I think it is a matter of preference, and also of the size of the file. If it is larger with more variables then I would tend to using the styles. prefix to disambiguate. You could also compromise and go for s = styles followed by s.headerHint. As to the name changing issue, you could always just write styles = newGangledStyles however most decent IDEs will ...


Only top voted, non community-wiki answers of a minimum length are eligible