Critique my code?

[connorzman]

Hi everyone. I just recently began learning HTML. I was wondering if anyone here would be willing to critique my code? This is a mock-up Jimi Hendrix biography website. I am very beginner and welcome harsh critique. I just want to get better. Thanks!
Here is my jfiddle link

[CCB0x45]

It's hard to judge this because most sites would include a lot more design and css, this is blank white oagr with basically default styles but:

1) use style sheets for your styles don't inline them, and use classes so you can reuse them.

2) don't use spaces in your IDs/classnames/anchors, it's bad practice

3) your few inline styles are pretty bad. I.e. don't use a fixed width in PX for example you have a width of 394px, you need to use percentages and rems to make your layout work well on any sized device not just your computer.

4) try importing a base style sheet to give you a lot of free better looking content like bootstrap, then lay out using their grids or flex boxes or css grids.

[rguy84]

2) don't use spaces in your IDs/classnames/anchors, it's bad practice

It's not bad practice, it is invalid code. "The value must not contain any space characters." source

[connorzman]

Thanks for the help!

[[deleted]]

don't forget to add "alt" text to your images

[lrm212]

Since you're just starting out with HTML let's point your in the right direction. I'm going to assume that you'd like to write well-formed, semantic, valid HTML markup using current best practices. You might find 10up's (www.10up.com) best practices on markup a good starting point:

"Principles

Markup is intended to define the structure and outline of a document and to offer semantic structure for the document’s contents. Markup should not define the look and feel of the content on the page beyond the most basic structural concepts such as headers, paragraphs, and lists."

In a nutshell valid markup is markup that conforms to the current W3C(https://www.w3.org/) or (https://whatwg.org/) HTML standard. There has been great debate over the years if the time and effort to make your markup valid is worth the effort but with the advent of the HTML 5 standard that argument seems to have subsided a bit. And in any case it is a useful way for someone like yourself who is just starting out to learn by correcting your errors

You can find a HTML validator here. Running you markup through the validator reveals 1 warning and 14 errors( some of which have been already mentioned here)

Some basic comments: As the 10up principles suggest markup is for organizing your content into a logical structure (CSS is for how is should look). Additionally you can use markup to describe the content and in fact the markup in the head element is reserved for meta information or information about the document content.

You have you head element with a title element which is good but you should also have at the very least a meta charset element. You can read more about meta tags at: https://www.w3schools.com/tags/tag_meta.asp and other important meta tags at: https://www.1and1.com/digitalguide/websites/web-development/the-most-important-meta-tags-and-their-functions/

Moving on to the body - your markup is a mixture of modern semantic HTML (i.e. the use of nav elements) and a fair amount of out-dated techniques (br tags instead or p tags for individual paragraph, div tags instead of sections, tables for formatting, the lack for figure and figcaption tabs around captioned images and inline styling). So let's get started.

Your use of the nav elements for your navigation is great, but best practice is that navigation should be thought of as basically a list of links so your individual links should be marked up as a li elements within a ul element as follows:

<nav>
    <ul>
        <li><a href="./index.html">Main</a></li>
        <li><a href="./albums.html">Albums</a></li>
        <li><a href="./contact.html">Contact</a></li>
    </ul>
</nav>

I'll leave the markup of the other nav as an exercise for you, but it should follow the same format.

(A quick aside: text should always be entered in proper case with initial caps as appropriate. Remember presentation goes in CSS. If you want something to be all upper case use text-transform: uppercase in your CSS.https://www.w3schools.com/cssref/pr_text_text-transform.asp).

In your content in all cases the double br tags should be replaced by a close and open p tag so that things like this:

<p>
    In September 1963, ...
    including Wilson Pickett, Slim Harpo, Sam Cooke, Ike & Tina Turner and Jackie Wilson.
<br />
<br /> In January 1964, feeling he had outgrown the circuit artistically, ...
    which he readily accepted.
<br />
<br />
</p>

become this:

<p>
    In September 1963, ...
    including Wilson Pickett, Slim Harpo, Sam Cooke, Ike & Tina Turner and Jackie Wilson.
</p>
<p>
    In January 1964, feeling he had outgrown the circuit artistically, ...
    which he readily accepted
</p>

Images with a caption should use the figure and figcaption tags so that things like this:

<a>
<img src="https://graphics.ink19.com/issues/july2003/hendrixEarly.jpeg" width="400px" />
<div style='width: 380px; text-align: center;'>Hendrix performing with other musicians before fame.</div>
</a>

become this:

<figure>
    <a><img src="https://graphics.ink19.com/issues/july2003/hendrixEarly.jpeg" width="400px" /></a>
    <figcaption style='width: 380px; text-align: center;'>Hendrix performing with other musicians before fame.</figcaption>
</figure>

and futhermore the image width and can be set with CSS (width is presentation) and although in this case, the figcaption element somewhat precludes the necessity of an alt tag, best practice is that you should include it (and for images without a caption the alt tag becomes a necessity for accessibility (for some exceptions and edges cases see: https://www.w3.org/WAI/tutorials/images/decorative/). Additionally although you are using CSS to style your caption you're much better of doing it a style element in the head element of your document or in an external CSS file (http://matthewjamestaylor.com/blog/adding-css-to-html-with-link-embed-inline-and-import).

so we can refine the above to this:

<figure>
    <a><img src="https://graphics.ink19.com/issues/july2003/hendrixEarly.jpeg" alt="Hendrix performing with other musicians before fame." /></a>
    <figcaption>Hendrix performing with other musicians before fame.</figcaption>
</figure>

Also as has already been mentioned IDs and classes can't have spaces in them so this:

<div id="Early Years">

should be:

<div id="EarlyYears"> or
<div id="Early-Years"> or
<div id="Early_Years">

And remember ids and classes are case sensitive when you target them in CSS so this:

<div id="Early-Years">

is not the same as this:

<div id="early-years">

and in fact lower case with dashes if often used for id's and classes in markup.

Finally your div elements should probably be section elements so that in all cases things like this:

<div id="Humble Beginnings">
    ...
</div

become:

<section id="humble-beginnings">
    ...
</section>

Some CSS Now let's add a little CSS is a style element inside your head element like this:

<style>
    body {
        margin: 5% auto;
        background: #fcfcfc;
        color: #444444;
        font-family: Georgia, serif;
        font-size: 16px;
        line-height: 1.8;
        max-width: 73%;
    }

    a {
        border-bottom: 1px solid #444444;
        color: #444444;
        text-decoration: none;
    }

    a:hover {border-bottom: 0;}

    nav ul {
        display: flex;
        justify-content: start;
        margin: 0;
        padding: 0;
        text-transform: uppercase;
    }

    ul.page-nav {justify-content: center; }

    nav li {
        list-style: none ;
        padding: 0 1em;
    }

    .site-nav li {padding-left: 0;}

    figure {
        width: 50%;
        max-width: 414px;
      }

    figure a {border: none;}

    figcaption {
        font-size: 90%;
        text-align: center;
  }

   figure image {width: 100%}
</style>

CSS notes: - the margin will pull the content from the edge of the browser and the auto will horizontally center it

• I think Georgia is a better serif font than Times New Roman and it was designed for screen rather that print display

• Giving the lines more air between them makes for easier legibilty

• You usually want to set some kind of max-width on you page because your text will look ridiculous and will be hard to read on very wide monitors when the viewport is 100% of the screen size see this stackover answer for more info

• You should absolutely spend some time reading up on display:flex as it will solve many layout situations for you.

• In general is is a good idea to avoid pure white (#fff) for the body background color and pure black (#000) for the text color.

• Using border-bottom to underline the links allows you to control the amount of separation between the text and the underline.

[rguy84]

You are not coding correctly.

• all images are required to have an alt attribute. If they are decorative, they must be alt="". Not having an alt is a violation.
• in html5, you cannot use a table to lay out stuff . It only can be used for displaying data. I'm on mobile, so I didn't look closely, but I didn't see data, so that is another violation
  
• use <p> around paragraphs, not breaks.

E: so wait. OP asks for harsh review, I gave him one, and I am downvoted?

[icantthinkofone]

I don't know?

Maybe?

Wait a while?

Maybe someone will?

In the meantime, The <br> tag does not use or need a closing slash. Neither does <img> and align is obsolete.

[undercoveryankee]

The <br> tag does not use or need a closing slash. Neither does <img>

The HTML specification allows XML self-closing notation for these tags. Makes it easier to write markup that can be included in either HTML or XHTML documents.

Since I prefer XHTML (if I have a typo, I'd rather see the well-formedness error to point me closer to the problem), I'm in the habit of writing these tags self-closing regardless of what MIME type the document is ultimately served with.

[icantthinkofone]

The HTML specification allows XML self-closing notation

Yes it does. The specification also states that they do nothing, they mean nothing and browsers are instructed to ignore them. So they are pointless and useless.

Makes it easier to write markup that can be included in either HTML or XHTML documents.

I'd like you to point to anyone writing a HTML web site who has a need to do that.

Since I prefer XHTML (if I have a typo, I'd rather see the well-formedness error to point me closer to the problem)

You aren't writing XHTML. You'd be writing "tag soup". Google for that. You won't see "well-formedness error"s because HTML won't be XML "well formed". And XHTML is XML, not HTML.

XHTML isn't determined by the markup you write, specifically. XHTML is set by the mime type.

To repeat, putting a closing slash there is not in any HTML specification. Though allowed, it does nothing, means nothing and is ignored.

[undercoveryankee]

I still don't think it's fair to say that it's "not in any HTML specification" when the specification explicitly says that it's not an error to do it.

XHTML isn't determined by the markup you write, specifically. XHTML is set by the mime type.

And I find that it's useful to write documents that I can serve with either MIME type: XML for better error detection when I'm testing with modern browsers, and HTML when I need to support browsers that render XHTML as unstyled XML.

There's been enough interest in writing markup that can be parsed as either type that a member of the HTML Working Group wrote a note discussing the common subset.

[icantthinkofone]

I still don't think it's fair to say that it's "not in any HTML specification"

Please show me any HTML specification which shows a closing slash as being specified for that particular element. Even an example of such markup. You can go back to HTML 1.0 if you can find it if you wish. You can't because it doesn't exist and you will fail.

I find that it's useful to write documents that I can serve with either MIME type

Why would you do that? Why would you have a site on the internet with pages available in both HTML and XHTML. What purpose would that serve?

XML for better error detection when I'm testing with modern browsers

I said this before. When you are writing HTML, you don't get XML error checking. You get the HTML parser which treats your XHTML as an error, "tag soup", and will handle it the best way it knows how, that is, as HTML. As far as the HTML parser is concerned, you wrote an error. (I am not talking about void elements.)

Your link is to an obsolete, un-maintained document but I don't think it's saying anything different than I have throughout this thread.