task 1 ready #4
task 1 ready #4
Conversation
There was a problem hiding this comment.
Congrats on submitting a solution to our newsletter task! 👍
I added a number of inline comments about different code issues that need to be resolved before this can be approved. Please feel free to comment on them if you need more explanation or would like to express a different opinion. I hope you find this informative. We're looking forward to seeing what you code next!
| @@ -3,6 +3,7 @@ | |||
| "version": "0.1.0", | |||
| "private": true, | |||
| "dependencies": { | |||
| "material-ui": "^0.20.1", | |||
There was a problem hiding this comment.
This is a correct way of adding a custom UI library to the project. That being said, it's best to focus on the React part of the task and only use some basic CSS styling. We should always aim at creating minimum effective pull requests. The more code we have, the harder it gets to manage it and maintain it.
Adding this also caused package-lock.json to be created which obscures the PR a little
| @@ -26,3 +26,19 @@ | |||
| from { transform: rotate(0deg); } | |||
| to { transform: rotate(360deg); } | |||
| } | |||
|
|
|||
| .table { | |||
There was a problem hiding this comment.
This style will apply to all the table elements in the project. This will also affect any other table that may be added in the future. Instead, you should use a class selector to differentiate the CarsDataTable elements from generic ones.
| </div> | ||
| <b>Your task is to render a table here</b> | ||
| <Table carsData={props.cars}/> | ||
| <Table2Mui carsData={props.cars}/> |
There was a problem hiding this comment.
The task was to create just a one table 😄
| <td>{ element.carMake }</td> | ||
| <td>{ element.carModel }</td> | ||
| <td>{ element.carModelYear }</td> | ||
| <td bgcolor={ element.bodyColor }>{ element.bodyColor }</td> |
There was a problem hiding this comment.
The bgcolor attribute is deprecated. You should use style here instead like so:
<td style={{ backgroundColor: element.bodyColor }} />Also the hex codes are not readable by a general user - we shouldn't display them.
| <td>{ element.carModel }</td> | ||
| <td>{ element.carModelYear }</td> | ||
| <td bgcolor={ element.bodyColor }>{ element.bodyColor }</td> | ||
| <td>{ element.dealerEmail }</td> |
There was a problem hiding this comment.
It would be great to have this email be a mailto: link.
|
|
||
| // Component write as a function | ||
|
|
||
| // const Table = (props) => { |
There was a problem hiding this comment.
Please don't submit commented code. You should just pick one approach and use it. In this case it seem that the functional component should be the way to go.
| constructor(props) { | ||
| super(props); | ||
| this.state = { | ||
| showCheckboxes: false, |
There was a problem hiding this comment.
Seems like there is nothing to change this state. Also this functionality, although interesting, wasn't a part of the task. We have more interesting tasks coming it, so it's best to focus entirely and exclusively at the current task at hand.
| <TableRowColumn>{ element.carMake }</TableRowColumn> | ||
| <TableRowColumn>{ element.carModel }</TableRowColumn> | ||
| <TableRowColumn>{ element.carModelYear }</TableRowColumn> | ||
| <TableRowColumn style={{ backgroundColor: `${element.bodyColor}` }}>{ element.bodyColor }</TableRowColumn> |
There was a problem hiding this comment.
Please note that there is no need to use string template literals like ${element.bodyColor} if you don't intend to append text - just do element.bodyColor
No description provided.