Skip to content

Conversation

@baspavlou
Copy link
Collaborator

No description provided.

import React, { Component } from 'react';
import request from 'request';
import { BrowserRouter as Router, Route, Link } from "react-router-dom";
import classnames from 'classnames';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

classnames is imported but it is never used

constructor(props) {
super(props);

var res = [];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fetching data in the constructor may result to rendering witout having all the data . This should be done in the ComponentDidMount method

super(props);

var res = [];
request('http://bidders.mytestcompany.com', function (error, response, body) {
Copy link
Collaborator Author

@baspavlou baspavlou Mar 8, 2020

Choose a reason for hiding this comment

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

Request package has been deprecated. Better use axios or fetch.

var live = [];
var created = [];

for (var i = 1; i < res.length; i++) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This loop does not include the last item of the array.

<hr className="bottom-spacing-2x top-spacing-2x" />
</div>
</div>
<div className="column small-12 medium-6 bottom-spacing-1x">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The two columns should be wrapped in a row so they are both in the same line.

<ul>
<li>
<a class="active" href="/bidders">
<Link to="/bidders" activeClass="active">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

activeClass is unnecessary

<li>
<a class="active" href="/bidders">
<Link to="/bidders" activeClass="active">
<i class="icon location_world"></i>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

class should be changed to className

<div className="column small-12 medium-6 bottom-spacing-1x">
<div data-testid="created-bidders-list" className="card" style={{height: '100%'}}>
<p className="card__title">1. Created </p>
{this.state.created.map(function (bidder) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both map functions should have a unique key id

Copy link
Collaborator Author

@baspavlou baspavlou left a comment

Choose a reason for hiding this comment

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

Please review the mentioned changes.

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.

3 participants