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

Baseline #9

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

Baseline #9

wants to merge 59 commits into from

Conversation

jlikesplants
Copy link

https://trello.com/b/I2rRvlyN/farmarrails

Experimented with online ERD generator- not totally sure about the connections it auto-generated. But it did help.
farmarrails

t.string "city"
t.string "county"
t.string "state"
t.integer "zip"
Copy link

Choose a reason for hiding this comment

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

What if the zipcode starts with 0? Zipcode should be stored as a string, because as an integer if there is a zero at as the first character it will be dropped.

@emgord
Copy link

emgord commented May 3, 2016

Really nice job on this project! You did a great job getting organized with your ERD diagram and making frequent and clear commits to track your progress. Your controllers are clear and you did a great job implementing basic CRUD methods and creating relationships between all your models. Try to keep all the logic in your models and controllers and avoid processing data in your views, and watch your instance vs. local variables in the controllers. Great job using partials as well!

width: 22rem;
text-align: left;
font-family: 'Lobster', cursive;
}
Copy link

@Dreedle Dreedle May 4, 2016

Choose a reason for hiding this comment

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

omg I love lobster. watch your indents in this file, though.

@Dreedle
Copy link

Dreedle commented May 4, 2016

I love the cabbages, and the look of the site, so much. (PS this is Audrey)

@market = Market.first
@vendor = Vendor.first
end

Copy link

@Dreedle Dreedle May 4, 2016

Choose a reason for hiding this comment

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

I can't figure out what @Market and @Vendor are used for in the index view. Maybe I'm just missing it- double check that you actually need this.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah- I think we put those there after developing another view that needed
them, realized we didn't for that one, and didn't take them back out.

On Tue, May 3, 2016 at 8:07 PM, Dreedle [email protected] wrote:

In app/controllers/markets_controller.rb
#9 (comment):

  • def create
  • @Market = Market.new(market_edit_params[:market])
  • if @market.save
  •  redirect_to markets_path
    
  • else
  •  render :new
    
  • end
  • end

I can't figure out what @Market https://github.com/market and @Vendor
https://github.com/vendor are used for in the view. Maybe I'm just
missing it- double check that you actually need this.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/Ada-C5/FarMarRails/pull/9/files/da2a02f087c82df712f5ab599b65813a6ffd05d7#r61986318

@jlikesplants
Copy link
Author

Aw, thanks!!! I love cabbages and lobster too :)

On Tue, May 3, 2016 at 7:58 PM, Dreedle [email protected] wrote:

I love the cabbages, and the look of the site, so much. (PS this is Audrey)


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#9 (comment)

@jlikesplants
Copy link
Author

Thanks so much for all your thoughtful and helpful comments! We definitely
debated where various pieces of logic should go, so that feedback feels
especially useful. And I definitely have a note somewhere to clarify where
to use instance vs local variables in this context, so thank you for that
as well. We appreciate your time!
<3
Justine

On Tue, May 3, 2016 at 4:07 PM, Emily Gordon [email protected]
wrote:

Really nice job on this project! You did a great job getting organized
with your ERD diagram and making frequent and clear commits to track your
progress. Your controllers are clear and you did a great job implementing
basic CRUD methods and creating relationships between all your models. Try
to keep all the logic in your models and controllers and avoid processing
data in your views, and watch your instance vs. local variables in the
controllers. Great job using partials as well!


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#9 (comment)

@vendor = Vendor.new
@market = Market.find(params["format"])
end

Copy link

Choose a reason for hiding this comment

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

Instead of doing things with 'format' in the params, you could use the relationship you set up between vendor and market and do @vendor.market when you need to refer to it's market. Seems simpler and then the route won't have a weird number after it in the url bar. Same goes for the other places you use 'format'.

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.

4 participants