Hot answers tagged

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

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 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

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

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 ...


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

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>


4

As a preface: Your code works good, is readable and does its job efficiently. The following review, though long, doesn't mean there is anything substantially wrong with it :-) General comments In the constructor, you're binding the event handlers to this. However, since you declare these event handlers as arrow functions, they are already bound to the ...


3

Fully functional example : class Calculator extends React.Component { constructor(props) { super(props) this.state = { currentNumber: '' } } addChar = char => ev => { if (!(this.state.currentNumber || ' ').endsWith(' ') || !char.endsWith(' ')){ this.setState(prevState => ({ currentNumber: prevState.currentNumber + char ...


3

A few things: I'm not sure why you need to map over the top level object? Are there top-level keys other than dataset that you're interested in? From your example, it looks like you only need the array of records (guests), in which case you don't need mapObject. I usually try to keep JSX expressions as slim as possible. Your JSX is quite long, and I can ...


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

React is component based, so i think you should not combine it, you should split component for the footer, component for the header, component for the container and if you want to render inside the container you can use Page.jsx ... render() { return ( <MainContainer> <Header/> {React.Children.only(this.props....


3

Main Question Can somebody please tell me what would have made my code more "correct"? It is difficult to know exactly what would have made my code more "correct" but it does look like the method handleClick is more complex than it needs to be. Specifically, there is a for loop that is not needed. Perhaps you were attempting to guard against cases where ...


3

You don't have to put content as a prop, you can do this: <section className={this.props.bgColor + " USBSection"> <div className="container">{this.props.children}</div> </section> and then you can use your component as: <MySection bgColor="bg-gray"> <MediaObject heading="today's rates!" /> <MediaObject ...


3

You very rarely need to use any in TypeScript because its type-system is so expressive. The problem is that copyFormat is specified to be any string. What would happen if you passed "blahblahblah" as copyFormat? Your types currently allow this!. Then, your code would attempt to do formats["blahblahblah"], but .blahblahblah is not a member of the formats ...


2

One thing that could help a lot is to realize that tests verify behavior, not implementation. This means you shouldn't be validating these things: should have a <form> element <form> element should have a onSubmit attribute onSubmit attribute should be of type function <form> element should have an <input /> element <input> ...


2

First of all, try to write code for pure components without using a class: It will be your App.js const App = ({ children }) => ( <main> {children} </main> ); export default App; It will be your Layout.js const Layout = ({ children, type }) => ( <div> type == 'wrap' ? ( <div> <Header /> ...


2

I find your code pretty good mostly. Some ideas for possible improvements: I don't think that PhaseForm should include the HideFormLink since PhaseForm handles the state for that and the form shouldn't need to know that it might be hidden. I would rather move that up to PhasesBox (eventually passing it as child down to PhaseForm if you need to have this ...


2

Works well! This: const liquidType = this.state.liquidType; const temperature = this.state.temperature; Can be written as: const { liquidType, temperature } = this.props; This: function LiquidState(props){ var liquid = props.liquidType; if (props.temperature >= liquidInventory[liquid].boiling){ return <p> Boiling {liquid}</p> } ...


2

Passing undefined in for cellRenderer should be the same as not passing it in at all. Because of this, the duplication can be removed by doing the following: {headers.map((header, i) => { return ( <Column label={header.label} dataKey={header.id} disableSort width={100} flexGrow={1} cellRenderer={header.index ?...


2

Yes - whenever the DOM is being queried (e.g. calls to document.querySelector(), etc.), that might likely be a sign that the Virtual DOM is not being utilized properly. Use the React component state for storing values for degree values The styles can be added inline such that the degree values of the transform-rotate styles reference values in the state: &...


2

Disclaimer: I know sqat about react.js. I only have some minor nitpicks. When the body of an anonymous function is a single line, instead of ... => { return value; }, you can write more compactly as ... => value, for example here: let updatedHobbies = this.state.hobbies.filter((hobby) => currentHobby !== hobby); Although this works, I think it's ...


2

There are a few things that I think are common practices in early 2018: First, call super with ...args, like in class Country extends Component { constructor(...args) { super(...args); // all the things } } Next, while I'm personally good with 4 occurrences of the same operation in this particular example, but if you detect a pattern and have ...


2

Ok, I had quite a few thoughts on this. State Your state seems more complex than it needs to be, and some of it is hiding outside of the state variable. I think all you really need to keep track of are: - What order the images are in - Which images are currently selected - Which images have been guessed correctly I don't really understand what ...


2

Yeah it's hard to read and only by looking at the source for RowFilter can you understand that it's injecting props into the child component that is given to it. Instead of passing it in as a child, you should pass it in a component prop. <RowFilter data={data} component={List} /> Then in RowFilter.js, the render simply looks like this. Note the ...


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