Tribute Page FCC - Stephen Hawking

Code Feedback
1

Hello Great People,

Just created my first project on FFC. Please review.

Thanks

2

Looks pretty good, and a great person to tribute.

A few things I might suggest:

First, using div tags all over the place, while acceptable, is painful. If you MUST use div tags, give them semantic class names. But, rather than divs within divs, consider:

  • using an article id="main" tag where you use div id="main"
  • within that, use a header to contain the header content (the title, the subtitle, the image)
  • also within the article, use section tags to indicate unique sections (much as you have div id="about", div id="tribute-info", you could use section id="about" or `section id=“tribute-info”).
  • use a footer to contain the stuff after your hr, and remove the hr.

They will function as divs, so there’s really no difference – but it is a much more accurate view of the page content: a tribute article, with a header, sections and a footer.

Second, you have an h3 tag nested in your ul – the only thing that can be in a ul are lis. Put the h3 before the ul. Personally, for this particular list, I might use a description list (dl tag). It is a great candidate for that, as each list item contains a dt (which you could use for your strong tag), and a dd (which would contain the rest of the entry).

It does look great, and it works well, but these are just some things to consider.

1 Like
3

That was really beautiful and constructive feedback. Thank you so much for your time.

I worked on your suggestion and refactored everything based on your feedback. I tried to convert every div into respective HTML5 tags and I found that the new tags are much better than only div(in terms of readability of code and of-course accessibility)

Once I converted the <li> to <dl> i faced problem in alignment so I tried with css-grid. and the result was incredible.(I still missing some alignment from left and right)

I really enjoyed the whole refactoring process and learned a lot. Thanks once again.

Have a look again.

1 Like
4

Only a few more suggestions. And that page looks INCREDIBLE. The changes are more in the way of guidelines than criticisms.

First, the main tag is redundant. Set the article as the root, and move the header and footer into that. Makes sense, as the header and footer content is about the article.

Second, the strong tags in each dt is redundant – simply set a font-weight on the dt itself within your CSS. The strong is cosmetic only, and isn’t really needed.

And lastly, in converting to HTML5 semantic tags, you may have missed one: you have an image div, when a more semantic tag might be:

<figure id="img-div">
  <img src="..." id="image" alt="">
  <figcaption id="img-caption">Stephen Hawking’s life is a triumph of intellect over adversity</figcaption>
</figure>

In using this, the div element can be replaced with something that makes sense to, for example, a screenreader.

And one last – the sub-heading in the header could just as easily be an h2 as a div. Again, it’s all about meaningful tags.

And I’m amazed by your refactor. Makes much more sense when viewing the HTML. And I’m glad you took the comments in the way I meant them – simply guidelines to make something that, when you put it in your portfolio, will wow prospective employers. Something to take pride in, that demonstrates a higher level of craftsmanship.

Phenomenal job.

5

Ahaa… Sweet. A nice and constructive feedback is like vitamins for me than criticisms. I really liked the way you helped me to refactor. This is 1st step of my long journey (29 more projects on FCC to do) and I hope you will help me like on rest of the projects.

  1. The reason I put main tag because the test case was complaining that the all other items should be in main id.
  2. font-weight trick is awesome. I will make that change. (I was feeling bad when I had to put strong tag so many time. Now I know how to get rid of it)
  3. Didn’t know there is something like figure tag. I will work on this too.

Currently working on second project (Survey form). Once done I will re-factor this project.

Thanks for the nice words. Its really motivating.

1 Like
6

See, though, they just want the proper id on an element. Doesn’t matter what the element actually IS. I’m using id="img-div" on a figure element, it isn’t even a div!

Glad my suggestions make sense. Seems to me you’ll go far. Best of luck!