amelafili / MI-449-SS18-740-css-frameworks-p1twXJ

0 stars 0 forks source link

Project Feedback #1

Open amelafili opened 6 years ago

amelafili commented 6 years ago

Build a homepage for an imaginary company (you can make up the product/service they offer)

@KatieMFritz Can you take a look at this? It's hosted here and meets the following criteria:

KatieMFritz commented 6 years ago

Hi Amela, nice job with your valid HTML and the content of your imaginary company homepage! ✨

This makes me hungry. Those potatoes look really good. 🥔

Here are your things to improve! 💪

Use Bootstrap 3

It looks like you're linking to the Bootstrap 4 alpha version, rather than Bootstrap 3: <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-alpha.6/css/bootstrap.min.css" integrity="sha384-rwoIResjU2yc3z8GV/NPeZWAv56rSmLldC3R/AZzGRnGxQQKnKkoFVhFQhNUwEyJ" crossorigin="anonymous">

You might need to tweak some of your classes once you switch to the Bootstrap 3 CSS. Read through the docs (make sure they're the 3 docs, not the 4 docs) and let me know if you have questions.

Only use Bootstrap 3

We didn't explicitly say not to use your own CSS, but I want you to style this only with Bootstrap so you get a feel for what it can do. Bootstrap is pretty powerful; I think it can do pretty much everything you're doing with inline styles! 👢 😄

Watch out for typos

While the validator didn't catch any invalid HTML, I'm seeing some inconsistent spacing, for example <img src ="http://goo.gl/0sX3jq" alt="Potato"> and <div class ="thumbnail">. This can cause things to break. Comb through your code for typos/extra spaces before you resubmit! 🔍

Formatting weirdness

One of the requirements is

Use the container class to center your page content

but I don't see the container class in your project! There's also some weird formatting on the bottom part of the screen for me - some missing images, and inconsistent margins, and the page is wider than my viewport. screenshot-amelafili github io-2018-01-12-19-37-53-971

Using Bootstrap 3 and the container class will help with some of the formatting. Once you've implemented that, go through the requirements again, carefully. I haven't checked the rest of the requirements yet, because your code will have to change anyway. Let me or Erik know if you have questions! 🚀

amelafili commented 6 years ago

I've updated the bootstrap but my formatting is still off, but i've changed it to bootstrap 3 and fixed some errors.

KatieMFritz commented 6 years ago

Cool. 😎 I can't approve it until it's all working, but I'm happy to help you get there if you ask me specific questions!

amelafili commented 6 years ago

I think I've corrected the container class issue!

KatieMFritz commented 6 years ago

Hi there! I can see that you're using Boostrap 3 now, and you've fixed some of the typos, but there are still problems with the way the site looks, especially the navbar:

screenshot-amelafili github io-2018-01-15-10-50-50-936

Some other issues:

Please slow down! 🐌 Before you submit any code review, I'd like you to:

  1. Review the project requirements, making sure you understand and have met each of them (ask if you're not sure).
  2. Review the comments from me or Erik and resolve each one of them.
  3. Get rid of any unused files and code in your repository.
  4. Check your project in the W3 validator and resolve errors and warnings.
  5. Look over your hosted site and make sure it looks professional and works the way you expect.
  6. Make sure your latest changes show up on all branches you are using (in this case, master and gh-pages).

These are some of the minimum expectations any employer or client will have for you when you are hired to create a website or application. We want you to get used to meeting them now, so you can do a great job in your career. 🚀

If you are confused about anything, I'm happy to help you if you ask me specific questions! That's literally my job. 😄

amelafili commented 6 years ago

I think I've solved all the problems, what were the unused files you were referring to?

KatieMFritz commented 6 years ago

This is getting A LOT closer. ✨ Nice job!

The unused files are the bootstrap folder and skeleton.css - you're linking to Bootstrap from their CDN (content delivery network - another website) and you're not using Skeleton on this project, so none of those files are necessary in your repo. 🙂

I'm still seeing some custom inline styles and inconsistent indentation, and the navbar doesn't look quite right. I'll come over and we can go through the file together. 😸

amelafili commented 6 years ago

I've fixed the issues! Thank you for your help! I've updated the navigation bar, changed the background color (kept getting an error on the validator for that line of code but knew no other way to change background color without using css) and i've gotten rid of the custom inline styles and i hope my indentation is fine.

KatieMFritz commented 6 years ago

Wow, your navbar looks great now! 🎉

Everything from the beginning to line 23 is solid. 😁 💪 As you pointed out, setting a background image with the background HTML property is no longer used, but if you've got your heart set on a grey background, I'll let it slide. 😉

After line 23, things get a little iffy still. You can also see on you hosted site that the layout seems off after the big image of mouthwatering roasted potatoes. 🥔 Let's take it a few lines at a time:

      <div class="thumbnail">
        <img src="http://del.h-cdn.co/assets/16/51/640x320/landscape-1482189029-delish-rosemary-roasted-potatoes-pin-2.jpg" alt="Potatoes" width="700" height="300"></div></div>

Thumbnail: The thumbnail class on your div is why the image is centered with the white border around it. It actually looks like the Jumbotron component in Bootstrap! If you want something to be large and centered like that in the future, I recommend using Jumbotron instead of thumbnail. If you like the way it looks now, you can keep the thumbnail class how it is. 📷

Closing the container class: See those two </div>s after your image? The first one closes <div class="thumbnail">, which is good. The second one closes <div class="container">, which is why the content that follows is all crammed to the left side of the page.

        <h1>The Automatic Potato Peeler</h1>
        <h2>How to Make Perfect Tater Tots</h2>

^These lines are fine, except for the indentation, which makes it look like they are nested inside <div class="thumbnail">. Move them one level to the left; they are actually siblings of your thumbnail div. ◀️


      <div class="row">
        <div class="col-sm-4"><p>Ever get tired of the work and time that goes into making your favorite potato meal? Well now for only $29.99, you and your family can make enjoying potatoes easier.</p></div>
        <div class="col-sm-8">Go from<img src="potatoes.jpg" class="img-thumbnail" alt="Original Potato" width="200" height="300">to<img src="tatertot.jpg" class="img-thumbnail" alt="Tater Tots" width="200" height="300">
      </div>
      </div>

^This is another situation where cleaning up the indentation and line breaks will help a lot! I'm going to start it for you, so it's easier to see what's going on. ✨

      <div class="row">
        <div class="col-sm-4">
          <p>Ever get tired of the work and time that goes into making your favorite potato meal? Well now for only $29.99, you and your family can make enjoying potatoes easier.</p>
        </div>
        <div class="col-sm-8">
          Go from
          <img src="potatoes.jpg" class="img-thumbnail" alt="Original Potato" width="200" height="300">
          to
          <img src="tatertot.jpg" class="img-thumbnail" alt="Tater Tots" width="200" height="300">
        </div>
      </div>

Once it's formatted properly, it's easier to see that it's valid HTML! Every <div> has a corresponding </div> and the code makes sense. Only two things to change:

Don't forget to add one more </div> to the end of the body to close your <div class="container">! 🐶


After you’ve made your changes and pushed them to GitHub and your hosted site, check carefully to make sure it looks right and passes the validator, then comment back here and I’ll take another look.

Thanks! :rocket:

amelafili commented 6 years ago

I've attempted to correct the issues! Thank you!

KatieMFritz commented 6 years ago

Hi Amela,

I'm getting some errors in the validator and when I look at your hosted site, I can tell that the stuff under the photo isn't inside the container class (because it's all squished on the left side). 🥔

Please fix that stuff, then resubmit and I'll look at the rest of your code. Be really thorough! I want to see you finish this project and move on, and I'm sure you do too. 😄

amelafili commented 6 years ago

Thank you! It should be fixed

KatieMFritz commented 6 years ago

YOU DID IT! 🎉 🎉 🎉 🥔 😸

Great job sticking with this! :shipit: