Skip to content

Product-List Eval#154

Open
JwThomas21 wants to merge 15 commits intoprojectshft:masterfrom
JwThomas21:master
Open

Product-List Eval#154
JwThomas21 wants to merge 15 commits intoprojectshft:masterfrom
JwThomas21:master

Conversation

@JwThomas21
Copy link

No description provided.

Copy link

@ronyrosenberg ronyrosenberg left a comment

Choose a reason for hiding this comment

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

Hi,
The app is not running. Please update the readme file with instructions on how to run and its missing a FE application. Please make these changes and resubmit the eval. Make sure to remove files from the node modules as they were pushed as well.

app.use(bodyParser.json());

// Connect to MongoDB
mongoose.connect('mongodb://127.0.0.1:27017/Products')

Choose a reason for hiding this comment

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

Important, use lowercase for the DB. Mongo is case sensitive.

Suggested change
mongoose.connect('mongodb://127.0.0.1:27017/Products')
mongoose.connect('mongodb://127.0.0.1:27017/products')

Comment on lines +3 to +4
const Product = require('../models/products');
const Review = require('../models/reviews');

Choose a reason for hiding this comment

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

convention says we should name our schema files in singular. Your schema represents one product and one review. Not many. Also, makes more sense.

Suggested change
const Product = require('../models/products');
const Review = require('../models/reviews');
const Product = require('../models/product');
const Review = require('../models/review');

Choose a reason for hiding this comment

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

Watch for empty folders

);
}

export default ProductList;

Choose a reason for hiding this comment

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

not in use

);
}

export default Product;

Choose a reason for hiding this comment

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

not in use

}
};

export default categoriesReducer;

Choose a reason for hiding this comment

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

duplicated name and doesn't seem to be in use

Comment on lines +46 to +71
<select
className="form-select"
value={sortByPrice}
onChange={(e) => {
setSortByPrice(e.target.value);
onSortChange(e.target.value);
}}
>
<option value="">Sort by Price</option>
<option value="lowest">Lowest to Highest</option>
<option value="highest">Highest to Lowest</option>
</select>
</div>
<div className="col-md-3">
<select
className="form-select"
value={selectedCategory}
onChange={(e) => {
setSelectedCategory(e.target.value);
onCategoryChange(e.target.value);
}}
>
<option value="">All Categories</option>
{categories.map(category => (
<option key={category} value={category}>{category}</option>
))}

Choose a reason for hiding this comment

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

DRY.
You could have created a select component that takes the options to reuse twice: once for categories and once for sorting.

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