-
Notifications
You must be signed in to change notification settings - Fork 14
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
Ampers: Maddie, Angela, Katie, Nora #25
base: master
Are you sure you want to change the base?
Conversation
…products table. edits product fixtures. adds tests to product_test.rb
…est for the create action
bEtsyWhat We're Looking For
Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it.Really wonderful job overall with this project! The site looks great and functions great, it's very complete. I didn't run into any major snags and it hits all of the objectives. Above all, the code looks pretty good; it's on average short and concise and dense. The tests look pretty good too. I've tried making small comments throughout the code when I can, but overall it's good. Just a weird small note: If I don't put a valid Photo URL value for a product I'm creating or editing as a merchant, it will break the views that try to render that product and its thumbnail. Oooh, NICE ABOUT PAGE Y'ALL! Again: good work; this project was a lot and you all coded, styled, coordinated, designed a lot. |
merchant = order_product.product.merchant | ||
if merchant == @current_merchant | ||
@order_products << order_product | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These each
loops that really just populate the @order_products
array could really be improved with an enumerable method that filters based on a boolean, like select
or find_all
|
||
def format_price(price) | ||
sprintf('%.2f', price) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of view helpers!
margin-top: 20px; | ||
} | ||
|
||
#ul_table li { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ul_table
is an interesting name for an ID: it doesn't really describe what's going on, besides that it's a table inside of a UL (then why wouldn't you use ul table
?), so it doesn't really help other developers who are looking at this.
Also in general, CSS ids/classes are hyphenated
bEtsy
Congratulations! You're submitting your assignment! These comprehension questions should be answered by all members of your team, not by a single teammate.
Comprehension Questions