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

Review #37

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

Review #37

wants to merge 33 commits into from

Conversation

DeathofaSellout
Copy link

No description provided.

@@ -49,12 +49,12 @@ app.get('/api', function apiIndex(req, res) {
res.json({
woopsIForgotToDocumentAllMyEndpoints: true, // CHANGE ME ;)
Copy link

Choose a reason for hiding this comment

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

Since you have updated all the below lines, you can change this flag to false. Or probably delete the whole line altogether so it does not show up.

Copy link

@achentha achentha left a comment

Choose a reason for hiding this comment

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

You just need to code up the app.js now to fill in the missing link.

Copy link

@mnfmnfm mnfmnfm left a comment

Choose a reason for hiding this comment

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

It seems like you didn't get to working code over the weekend, which is concerning to me.


var db = require('../models');

// // GET /api/skatespots
Copy link

Choose a reason for hiding this comment

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

It seems like this code is mostly-working, but all commented out--I'm confused by the state this code was left in.


// var new_campsite = {description: "Sharp rocks. Middle of nowhere."}
var new_skatepark = {description: "Sharp rocks. Middle of nowhere."}
Copy link

Choose a reason for hiding this comment

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

It seems like you didn't actually edit your seed file to be relevant to the structure of your skatespots.

<div class="row"> <!-- a row just sets up the containters -->
<!-- offset is spacing, offest by one leaves the 1st container empty
and starts the div one over from the left, since there are 10
it centers it, leaving one column empty on both sides -->
Copy link

Choose a reason for hiding this comment

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

These comments are awesome!

<!-- offset is spacing, offest by one leaves the 1st container empty
and starts the div one over from the left, since there are 10
it centers it, leaving one column empty on both sides -->
<div class="col-lg-10 col-lg-offset-1">
Copy link

Choose a reason for hiding this comment

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

I'm concerned that you're using col-lg in some places, and col-md in others--you generally want to use the smallest possible size for everything, so make all of them col-md or col-sm.

@DeathofaSellout
Copy link
Author

DeathofaSellout commented Sep 7, 2017 via email

@mnfmnfm
Copy link

mnfmnfm commented Sep 7, 2017

Using col-lg will only apply once the screen is at least 992px. It should have no visual difference, once the screen is that size, between if you used col-lg or col-md. (If that's the look you were going for, my apologies!)

Also, it's true that we have used two different basic file structures in setting up our apps. I do think you're in a reasonable place, in terms of those setup steps, but I'm happy to talk more about this in person too. We will definitely continue to use models, controllers, view code, frontend JS, CSS files, and all the rest going forward; all the other differences between projects are minor, and are there to show you that there isn't just one way of making a project work.

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