Skip to content
This repository was archived by the owner on Oct 26, 2020. It is now read-only.

Daniel carter week 2 website #81

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

Conversation

carterd888
Copy link

made a test comment

@carterd888
Copy link
Author

@Codeama hopefully this one will work

@EmilePW
Copy link

EmilePW commented Jun 7, 2020

Hi @carterd888, I see what the issue with your previous pull request was. It's just a small problem with the file paths to your CSS and image files. The error happens because I opened the HTML file for your website directly into my browser where the URL is file:///home/emile/code/HTML-CSS-Homework/week2/2-website/index.html in my case. When you then e.g. try and load the CSS by beginning with / it will look at the base of my file system instead of at the base of your folder as you intended. In a different context this might not cause an error, but it's good to be aware of these potential downfalls when using relative links. When I, for example, add https in front of the links to resources from the web and use ./ in front of the CSS links to refer to the current folder, then everything works fine.

Here's a screenshot of my console when I first loaded the site:

image

If you're unsure of anything around absolute/relative links, here's a good primer from MDN: https://developer.mozilla.org/en-US/docs/Learn/HTML/Introduction_to_HTML/Creating_hyperlinks

On the website, overall I think you've done a good job with a few small issues:

Pros

  • Design quite closely resembles the spec
  • Correct use of semantic HTML elements

Cons

  • the background image doesn't cover the page at extra large screen sizes
  • ids and classes are using camel case instead of the convention of separating words with hyphens, i.e. you should use "introducing-karma" instead of "introducingKarma" (this is camel case and is more commonly used in JavaScript)
  • For the items underneath "Everyone needs a little Karma", each item needs to be wrapped in an element to contain the icon and the text together where as you have the icons on one row and then the text on another row

i.e. it should be structured like:

<div>
  <icon>
  <text>
</div>
<div>
  <icon>
  <text>
</div>
<div>
  <icon>
  <text>
</div>

instead of

<div>
  <icon>
  <icon>
  <icon>
</div>
<div>
  <text>
  <text>
  <text>
</div>

This allows you contain and style the icon and text together.

There are a few small inconsistencies with the design spec but they are very small things like the logo being a slightly different size.

Like I said though it's a good job overall - hope that feedback is helpful! 👍

@Codeama
Copy link

Codeama commented Jun 8, 2020

@Codeama hopefully this one will work

It's indeed a relative path issue @carterd888. Thanks for spotting that @EmilePW. So try and update your CSS href file paths in your html and css files. That would take care of looking in the right directory when on a different operating system. Also there is no need to focus on responsiveness for this homework. Aim is to try to produce a very close copy of the original website. So if you're doing it on a desktop, focus on rendering it to look similar to the original on a desktop screen size too (which you've done). As EmilePW said, I think you've done a good job. Just a few things to update that are mostly styling. Well done!

}

#introducingKarma {
background-image: url("/2-website/img/first-background.jpg");
Copy link

Choose a reason for hiding this comment

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

Update the path to "../img/first-background.jpg"

@@ -8,14 +9,62 @@
<link href="//maxcdn.bootstrapcdn.com/font-awesome/4.2.0/css/font-awesome.min.css" rel="stylesheet">
<link rel="stylesheet" href="css/normalize.css">
<!-- Add a link to your CSS file here (use the line above to guide you) -->
<link rel="stylesheet" href="css/style.css">
<link rel="shortcut icon" type="image/x-icon" href="favicon.ico">
<link rel="stylesheet" href="/2-website/css/style.css">
Copy link

Choose a reason for hiding this comment

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

Update this to: "./css/style.css"

Comment on lines 40 to 42
<img id="device" src="img/icon-devices.svg" alt="device logo">
<img id="coffee" src="img/icon-coffee.svg" alt="coffee logo">
<img id="refill" src="img/icon-refill.svg" alt="device logo">
Copy link

Choose a reason for hiding this comment

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

And these too as suggested above.

<header>
<nav>
<div class="navigation-container">
<img id="karma-logo" src="img/karma-logo.svg" alt="karma logo">
Copy link

Choose a reason for hiding this comment

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

Same as above.

@carterd888
Copy link
Author

Hi @EmilePW and @Codeama,

i have made changes as suggested,

thanks again for the feedback!

@Codeama
Copy link

Codeama commented Jun 8, 2020

It's all good now, Daniel. :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants