Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React random colors #23

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vikisimovska
Copy link

@vikisimovska vikisimovska commented Dec 26, 2017

Adding solution for Random Colors.

Copy link
Contributor

@hueter hueter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vikisimovska I'd like you to refactor this a bit to do away with the Boxes component and generate the array from 1 to 24 more programmatically.

super(props);

this.state = {
numbers : [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😩

Array(24).fill(0).map((e, i) => i + 1);

😄

this.setState({
numbers : newArr
})
}, 300);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is 300 supposed to do here?

@@ -0,0 +1,12 @@
import React, { Component } from 'react';

class Box extends Component{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since there's no state it is preferable to use a stateless functional component

import React, { Component } from 'react';
import Box from './box';

class Boxes extends Component{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having this Boxes component is a bit of an anti-pattern. You don't really need a component whose only purpose is to render x number of a different component.


render(){
const boxList = this.props.numbers.map((num, idx) => {
return(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this logic into <App />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants