Simple color card built with react

Code Feedback
1

I would like some feedback on this project.

2

Cool looking color component =). Here’s some feedback for you to consider:

  • Try to be consistent with indentation (and formatting in general). E.g., everything starting on CArs.js line 19 appears to be indented an extra level. In some cases, you’re using 2-spaces for indent, in others 4 spaces. Consistent formatting makes it easier to read and easier to identify the scope of different code blocks.
  • There are also some inconsistencies in use of semicolons. I would recommend using a tool like eslint, or preferably standard, to enforce consistent styling (they can also help identify dangerous code constructs that may not do what you expect).
  • You can inline the export keyword (or export default as appropriate) with a class declaration. E.g., in App.js, instead of:
class App extends Component { /* ... */ }
export default App

You can use:

export default class App extends Component { /* ... */ }
  • In your Card class, I would recommend setting up changeColor as a bound method in the constructor, instead of in the render method. The bind method will create a new Function object every time it’s called, so you’ll actually be generating different results from render every time it’s called, even if your state hasn’t changed. This can lead to performance issues and is generally considered an anti-pattern (a component’s render method should be a pure function of the component’s props and state, so if neither have changed, it should return an identical result). There’s some more discussion on this page of the React documentation. In your case, it could look like this:
class Card extends Component {
    constructor(props) {
        super(props);
        // ...
        this.changeColor = this.changeColor.bind(this);
    }
    // ...
    render() {
        // ...
        /* ... */ onChange={this.changeColor} >
        // ...
    }
}
  • I don’t think I would have created separate components for Swatch and Label, I would probably stick with just inlining the HTML Dom elements directly in the Card::render method. It reduces complexity a bit and makes it easier for someone to read through (fewer modules and classes to keep track of). It would also allow you to put the onChange handler directly on the input element, which is probably where it really wants to be. If some modification in the future adds additional complexity to either of these child elements, they can always be pulled out into separate components then, but as of now, they don’t really offer much over the simple solution.
1 Like
3

Thanks a lot for taking the time to give detailed feedback.